Merge pull request #8155 from rabbitmq/fix-feature-flags-init-vs-code_server-deadlock-take2

rabbit_feature_flags: Fix possible deadlock when calling the Code server (take 2)

Author: dumbbell

deps/rabbit/app.bzl

@@ -114,6 +114,7 @@ def all_beam_files(name = "all_beam_files"):
             "src/rabbit_ff_extra.erl",
             "src/rabbit_ff_registry.erl",
             "src/rabbit_ff_registry_factory.erl",
+            "src/rabbit_ff_registry_wrapper.erl",
             "src/rabbit_fhc_helpers.erl",
             "src/rabbit_fifo.erl",
             "src/rabbit_fifo_client.erl",
@@ -355,6 +356,7 @@ def all_test_beam_files(name = "all_test_beam_files"):
             "src/rabbit_ff_extra.erl",
             "src/rabbit_ff_registry.erl",
             "src/rabbit_ff_registry_factory.erl",
+            "src/rabbit_ff_registry_wrapper.erl",
             "src/rabbit_fhc_helpers.erl",
             "src/rabbit_fifo.erl",
             "src/rabbit_fifo_client.erl",
@@ -609,6 +611,7 @@ def all_srcs(name = "all_srcs"):
             "src/rabbit_ff_extra.erl",
             "src/rabbit_ff_registry.erl",
             "src/rabbit_ff_registry_factory.erl",
+            "src/rabbit_ff_registry_wrapper.erl",
             "src/rabbit_fhc_helpers.erl",
             "src/rabbit_fifo.erl",
             "src/rabbit_fifo_client.erl",

deps/rabbit/src/rabbit_feature_flags.erl

@@ -113,7 +113,7 @@
          remote_nodes/0,
          running_remote_nodes/0,
          does_node_support/3,
-         inject_test_feature_flags/1,
+         inject_test_feature_flags/1, inject_test_feature_flags/2,
          query_supported_feature_flags/0,
          does_enabled_feature_flags_list_file_exist/0,
          read_enabled_feature_flags_list/0,
@@ -336,8 +336,8 @@ list() -> list(all).
 %% `disabled'.
 %% @returns A map of selected feature flags.
 
-list(all)      -> rabbit_ff_registry:list(all);
-list(enabled)  -> rabbit_ff_registry:list(enabled);
+list(all)      -> rabbit_ff_registry_wrapper:list(all);
+list(enabled)  -> rabbit_ff_registry_wrapper:list(enabled);
 list(disabled) -> maps:filter(
                     fun(FeatureName, _) -> is_disabled(FeatureName) end,
                     list(all)).
@@ -381,7 +381,7 @@ enable(FeatureNames) when is_list(FeatureNames) ->
     with_feature_flags(FeatureNames, fun enable/1).
 
 uses_callbacks(FeatureName) when is_atom(FeatureName) ->
-    case rabbit_ff_registry:get(FeatureName) of
+    case rabbit_ff_registry_wrapper:get(FeatureName) of
         undefined    -> false;
         FeatureProps -> uses_callbacks(FeatureProps)
     end;
@@ -504,9 +504,11 @@ is_supported(FeatureNames, Timeout) ->
 %%   `false' if one of them is not.
 
 is_supported_locally(FeatureName) when is_atom(FeatureName) ->
-    rabbit_ff_registry:is_supported(FeatureName);
+    rabbit_ff_registry_wrapper:is_supported(FeatureName);
 is_supported_locally(FeatureNames) when is_list(FeatureNames) ->
-    lists:all(fun(F) -> rabbit_ff_registry:is_supported(F) end, FeatureNames).
+    lists:all(
+      fun(F) -> rabbit_ff_registry_wrapper:is_supported(F) end,
+      FeatureNames).
 
 -spec is_enabled(feature_name() | [feature_name()]) -> boolean().
 %% @doc
@@ -563,19 +565,19 @@ is_enabled(FeatureNames, blocking) ->
     end.
 
 is_enabled_nb(FeatureName) when is_atom(FeatureName) ->
-    rabbit_ff_registry:is_enabled(FeatureName);
+    rabbit_ff_registry_wrapper:is_enabled(FeatureName);
 is_enabled_nb(FeatureNames) when is_list(FeatureNames) ->
     lists:foldl(
       fun
           (_F, state_changing = Acc) ->
               Acc;
           (F, false = Acc) ->
-              case rabbit_ff_registry:is_enabled(F) of
+              case rabbit_ff_registry_wrapper:is_enabled(F) of
                   state_changing -> state_changing;
                   _              -> Acc
               end;
           (F, _) ->
-              rabbit_ff_registry:is_enabled(F)
+              rabbit_ff_registry_wrapper:is_enabled(F)
       end,
       true, FeatureNames).
 
@@ -710,7 +712,7 @@ get_state(FeatureName) when is_atom(FeatureName) ->
 %% given feature flag name doesn't correspond to a known feature flag.
 
 get_stability(FeatureName) when is_atom(FeatureName) ->
-    case rabbit_ff_registry:get(FeatureName) of
+    case rabbit_ff_registry_wrapper:get(FeatureName) of
         undefined    -> undefined;
         FeatureProps -> get_stability(FeatureProps)
     end;
@@ -738,6 +740,9 @@ init() ->
 -define(PT_TESTSUITE_ATTRS, {?MODULE, testsuite_feature_flags_attrs}).
 
 inject_test_feature_flags(FeatureFlags) ->
+    inject_test_feature_flags(FeatureFlags, true).
+
+inject_test_feature_flags(FeatureFlags, InitReg) ->
     ExistingAppAttrs = module_attributes_from_testsuite(),
     FeatureFlagsPerApp0 = lists:foldl(
                             fun({Origin, Origin, FFlags}, Acc) ->
@@ -768,7 +773,10 @@ inject_test_feature_flags(FeatureFlags) ->
       [FeatureFlags, AttributesFromTestsuite],
       #{domain => ?RMQLOG_DOMAIN_FEAT_FLAGS}),
     ok = persistent_term:put(?PT_TESTSUITE_ATTRS, AttributesFromTestsuite),
-    rabbit_ff_registry_factory:initialize_registry().
+    case InitReg of
+        true  -> rabbit_ff_registry_factory:initialize_registry();
+        false -> ok
+    end.
 
 module_attributes_from_testsuite() ->
     persistent_term:get(?PT_TESTSUITE_ATTRS, []).

deps/rabbit/src/rabbit_ff_controller.erl

@@ -1387,7 +1387,7 @@ enable_dependencies1(
       Rets :: #{node() => term()}.
 
 run_callback(Nodes, FeatureName, Command, Extra, Timeout) ->
-    FeatureProps = rabbit_ff_registry:get(FeatureName),
+    FeatureProps = rabbit_ff_registry_wrapper:get(FeatureName),
     Callbacks = maps:get(callbacks, FeatureProps, #{}),
     case Callbacks of
         #{Command := {CallbackMod, CallbackFun}}

deps/rabbit/src/rabbit_ff_registry.erl

@@ -37,22 +37,26 @@
 -on_load(on_load/0).
 -endif.
 
-%% Initially, is_registry_initialized/0 always returns false and this `Call'
-%% is always called. The case statement is here to convince Dialyzer that the
-%% function could return values of type `__ReturnedIfUninitialized' or
+%% In this registry stub, most functions want to return `init_required' to let
+%% {@link rabbit_ff_registry_wrapper} do the first time initialization.
+%%
+%% The inner case statement is here to convince Dialyzer that the function
+%% could return values of type `__ReturnedIfUninitialized' or
 %% `__NeverReturned'.
 %%
 %% If the function was only calling itself (`Call'), Dialyzer would consider
-%% that it would never return.
+%% that it would never return. With the outer case, Dialyzer would conclude
+%% that `__ReturnedIfUninitialized' is always returned and other values will
+%% never be returned and there is no point in expecting them.
 %%
-%% With just `is_registry_initialized()' case, Dialyzer would conclude that
-%% `__ReturnedIfUninitialized' is always returned and other values will never
-%% be returned and there is no point in expecting them.
+%% In the end, `Call' is never executed because {@link
+%% rabbit_ff_registry_wrapper} is responsible for calling the registry
+%% function again after initialization.
 %%
 %% With both cases in place, it seems that we can convince Dialyzer that the
 %% function returns values matching its spec.
 -define(convince_dialyzer(__Call, __ReturnedIfUninitialized, __NeverReturned),
-        case is_registry_initialized() of
+        case always_return_true() of
             false ->
                 __Call;
             true ->
@@ -62,8 +66,11 @@
                 end
         end).
 
--spec get(rabbit_feature_flags:feature_name()) ->
-    rabbit_feature_flags:feature_props_extended() | undefined.
+-spec get(FeatureName) -> Ret when
+      FeatureName :: rabbit_feature_flags:feature_name(),
+      Ret :: FeatureProps | init_required,
+      FeatureProps :: rabbit_feature_flags:feature_props_extended() |
+                      undefined.
 %% @doc
 %% Returns the properties of a feature flag.
 %%
@@ -74,13 +81,15 @@
 %% @returns the properties of the specified feature flag.
 
 get(FeatureName) ->
-    _ = rabbit_ff_registry_factory:initialize_registry(),
     ?convince_dialyzer(
        ?MODULE:get(FeatureName),
-       undefined,
+       init_required,
        #{provided_by => rabbit}).
 
--spec list(all | enabled | disabled) -> rabbit_feature_flags:feature_flags().
+-spec list(Which) -> Ret when
+      Which :: all | enabled | disabled,
+      Ret :: FeatureFlags | init_required,
+      FeatureFlags :: rabbit_feature_flags:feature_flags().
 %% @doc
 %% Lists all, enabled or disabled feature flags, depending on the argument.
 %%
@@ -92,10 +101,11 @@ get(FeatureName) ->
 %% @returns A map of selected feature flags.
 
 list(Which) ->
-    _ = rabbit_ff_registry_factory:initialize_registry(),
-    ?convince_dialyzer(?MODULE:list(Which), #{}, #{}).
+    ?convince_dialyzer(?MODULE:list(Which), init_required, #{}).
 
--spec states() -> rabbit_feature_flags:feature_states().
+-spec states() -> Ret when
+      Ret :: FeatureStates | init_required,
+      FeatureStates :: rabbit_feature_flags:feature_states().
 %% @doc
 %% Returns the states of supported feature flags.
 %%
@@ -105,10 +115,12 @@ list(Which) ->
 %% @returns A map of feature flag states.
 
 states() ->
-    _ = rabbit_ff_registry_factory:initialize_registry(),
-    ?convince_dialyzer(?MODULE:states(), #{}, #{}).
+    ?convince_dialyzer(?MODULE:states(), init_required, #{}).
 
--spec is_supported(rabbit_feature_flags:feature_name()) -> boolean().
+-spec is_supported(FeatureName) -> Ret when
+      FeatureName :: rabbit_feature_flags:feature_name(),
+      Ret :: Supported | init_required,
+      Supported :: boolean().
 %% @doc
 %% Returns if a feature flag is supported.
 %%
@@ -120,10 +132,12 @@ states() ->
 %%   otherwise.
 
 is_supported(FeatureName) ->
-    _ = rabbit_ff_registry_factory:initialize_registry(),
-    ?convince_dialyzer(?MODULE:is_supported(FeatureName), false, true).
+    ?convince_dialyzer(?MODULE:is_supported(FeatureName), init_required, true).
 
--spec is_enabled(rabbit_feature_flags:feature_name()) -> boolean() | state_changing.
+-spec is_enabled(FeatureName) -> Ret when
+      FeatureName :: rabbit_feature_flags:feature_name(),
+      Ret :: Enabled | init_required,
+      Enabled :: boolean() | state_changing.
 %% @doc
 %% Returns if a feature flag is enabled or if its state is changing.
 %%
@@ -135,8 +149,7 @@ is_supported(FeatureName) ->
 %%   its state is transient, or `false' otherwise.
 
 is_enabled(FeatureName) ->
-    _ = rabbit_ff_registry_factory:initialize_registry(),
-    ?convince_dialyzer(?MODULE:is_enabled(FeatureName), false, true).
+    ?convince_dialyzer(?MODULE:is_enabled(FeatureName), init_required, true).
 
 -spec is_registry_initialized() -> boolean().
 %% @doc
@@ -169,7 +182,9 @@ is_registry_initialized() ->
 is_registry_written_to_disk() ->
     always_return_true().
 
--spec inventory() -> rabbit_feature_flags:inventory().
+-spec inventory() -> Ret when
+      Ret :: Inventory | init_required,
+      Inventory :: rabbit_feature_flags:inventory().
 
 inventory() ->
     _ = rabbit_ff_registry_factory:initialize_registry(),

deps/rabbit/src/rabbit_ff_registry_factory.erl

@@ -735,14 +735,9 @@ registry_vsn() ->
     proplists:get_value(vsn, Attrs, undefined).
 
 purge_old_registry(Mod) ->
-    case erlang:check_process_code(self(), rabbit_ff_registry) of
-        false ->
-            case code:is_loaded(Mod) of
-                {file, _} -> do_purge_old_registry(Mod);
-                false     -> ok
-            end;
-        true ->
-            ok
+    case code:is_loaded(Mod) of
+        {file, _} -> do_purge_old_registry(Mod);
+        false     -> ok
     end.
 
 do_purge_old_registry(Mod) ->