rabbit: Run plugins' boot steps during rabbit start/2

[dumbbell]

This restores the behavior prior the commit making rabbit closer to a standard Erlang application.

Plugins are still actually started after rabbit is started (because they depend on the rabbit application). Only the execution of their boot steps was moved earlier.

With the behavior restored, it also means that a plugin's dependencies are not started yet when its boot steps are executed.

[michaelklishin]

So what good does this make if a plugin boot step cannot rely on any of its dependencies?

[dumbbell]

That's the old behavior (the one still in 3.7.x), so if plugin works with it, they should work with this patch.

The new behavior fixed that (dependencies were loaded/had their boot steps exeucted/started before a plugin was taken care of), but they are incompatible, so we have to choose between the two.

[lukebakken]

I'm going to test to see if the changes in this PR resolve the issue reported by @noxdafox

https://groups.google.com/g/rabbitmq-users/c/m7jgGmIk4oY/m/4tmLsVM0AwAJ

[noxdafox]

Thanks @lukebakken for pinging me.

Please correct me if this is not the right place for this conversation.

From a Plugin development POW I would expect to be able to hook into the broker bootstrap procedure to be able to configure its internals based on my needs:

• Register my exchanges
• Override queue behaviour implementation modules
• Start any resource needed to be up early

Then, I would wait for my Erlang application (if any) to be started at last. As @dumbbell said, my app depends on rabbit to be started so it should not be started before.

It is valuable to have this pre and post bootstrapping phases as it gives me more control on how to properly setup my Plugin around the broker.

[lukebakken]

Associated PRs:

• Original refactor leading to this issue - Make RabbitMQ a regular Erlang/OTP application #2180
• Issue that may be resolved by rabbit: Run plugins' boot steps during rabbit start/2 #2656 - rabbitmq-server failed to start after enabling the rabbitmq_email plugin gotthardp/rabbitmq-email#42

[lukebakken]

Hm, the error reported in gotthardp/rabbitmq-email#42 happens with this PR (#2656) as well as the current master code in RabbitMQ:

BOOT FAILED
===========
Error during startup: {error,
                       {rabbitmq_email,
                        {{shutdown,
                          {failed_to_start_child,message_handler_sup,
                           {shutdown,
                            {failed_to_start_child,
                             'message_handlerexample.com',
                             {{badmatch,{error,broker_not_found_on_node}},
                              [{rabbit_message_handler,init,1,
                                [{file,"src/rabbit_message_handler.erl"},
                                 {line,25}]},
                               {gen_server,init_it,2,
                                [{file,"gen_server.erl"},{line,417}]},
                               {gen_server,init_it,6,
                                [{file,"gen_server.erl"},{line,385}]},
                               {proc_lib,init_p_do_apply,3,
                                [{file,"proc_lib.erl"},{line,226}]}]}}}}},
                         {rabbitmq_email_app,start,[normal,[]]}}}}

The reason being that on plugin init it tries to start an AMQP direct connection which fails with that error. I'm going to continue investigating (cc @dumbbell @michaelklishin).

[lukebakken]

@noxdafox @dumbbell my commit d2f0af4 should start plugins after the rabbit application is fully started. I'm going to test to see if this addresses the rabbitmq-message-deduplication issue as well.

[noxdafox]

@lukebakken please be wary that the last version of the plugin already addresses this issue. Therefore to reproduce it, you should use a less recent version.

Please let me know if you encounter issues while testing.

[lukebakken]

@noxdafox thank you for the heads up! I'm sure I would have missed that.

[dumbbell]

Here is a patch adding a core_started state after rabbit is started, but before plugins are started:

diff --git a/deps/rabbit/apps/rabbitmq_prelaunch/src/rabbit_boot_state.erl b/deps/rabbit/apps/rabbitmq_prelaunch/src/rabbit_boot_state.erl
index c76824e7be..12a0e1f192 100644
--- a/deps/rabbit/apps/rabbitmq_prelaunch/src/rabbit_boot_state.erl
+++ b/deps/rabbit/apps/rabbitmq_prelaunch/src/rabbit_boot_state.erl
@@ -12,11 +12,13 @@
 
 -export([get/0,
          set/1,
-         wait_for/2]).
+         wait_for/2,
+         has_reached/1,
+         has_reached_and_is_active/1]).
 
 -define(PT_KEY_BOOT_STATE,    {?MODULE, boot_state}).
 
--type boot_state() :: 'stopped' | 'booting' | 'ready' | 'stopping'.
+-type boot_state() :: 'stopped' | 'booting' | 'core_started' | 'ready' | 'stopping'.
 
 -export_type([boot_state/0]).
 
@@ -36,7 +38,7 @@ set(BootState) ->
 
 -spec wait_for(boot_state(), timeout()) -> ok | {error, timeout}.
 wait_for(BootState, infinity) ->
-    case is_reached(BootState) of
+    case has_reached(BootState) of
         true  -> ok;
         false -> Wait = 200,
                  timer:sleep(Wait),
@@ -44,7 +46,7 @@ wait_for(BootState, infinity) ->
     end;
 wait_for(BootState, Timeout)
   when is_integer(Timeout) andalso Timeout >= 0 ->
-    case is_reached(BootState) of
+    case has_reached(BootState) of
         true  -> ok;
         false -> Wait = 200,
                  timer:sleep(Wait),
@@ -53,24 +55,35 @@ wait_for(BootState, Timeout)
 wait_for(_, _) ->
     {error, timeout}.
 
-boot_state_idx(stopped)  -> 0;
-boot_state_idx(booting)  -> 1;
-boot_state_idx(ready)    -> 2;
-boot_state_idx(stopping) -> 3.
+boot_state_idx(stopped)      -> 0;
+boot_state_idx(booting)      -> 1;
+boot_state_idx(core_started) -> 2;
+boot_state_idx(ready)        -> 3;
+boot_state_idx(stopping)     -> 4.
 
 is_valid(BootState) ->
     is_integer(boot_state_idx(BootState)).
 
-is_reached(TargetBootState) ->
-    is_reached(?MODULE:get(), TargetBootState).
+has_reached(TargetBootState) ->
+    has_reached(?MODULE:get(), TargetBootState).
 
-is_reached(CurrentBootState, CurrentBootState) ->
+has_reached(CurrentBootState, CurrentBootState) ->
     true;
-is_reached(stopping, stopped) ->
+has_reached(stopping, stopped) ->
     false;
-is_reached(_CurrentBootState, stopped) ->
+has_reached(_CurrentBootState, stopped) ->
     true;
-is_reached(stopped, _TargetBootState) ->
+has_reached(stopped, _TargetBootState) ->
     true;
-is_reached(CurrentBootState, TargetBootState) ->
+has_reached(CurrentBootState, TargetBootState) ->
     boot_state_idx(TargetBootState) =< boot_state_idx(CurrentBootState).
+
+has_reached_and_is_active(TargetBootState) ->
+    case ?MODULE:get() of
+        stopped ->
+            false;
+        CurrentBootState ->
+            has_reached(CurrentBootState, TargetBootState)
+            andalso
+            not has_reached(CurrentBootState, stopping)
+    end.
diff --git a/deps/rabbit/src/rabbit.erl b/deps/rabbit/src/rabbit.erl
index 3ccfbe0e3a..80c114e40d 100644
--- a/deps/rabbit/src/rabbit.erl
+++ b/deps/rabbit/src/rabbit.erl
@@ -884,7 +884,7 @@ start(normal, []) ->
         %% We can load all plugins and refresh their feature flags at
         %% once, because it does not involve running code from the
         %% plugins.
-        app_utils:load_applications(Plugins),
+        ok = app_utils:load_applications(Plugins),
         ok = rabbit_feature_flags:refresh_feature_flags_after_app_load(
                Plugins),
 
@@ -893,6 +893,7 @@ start(normal, []) ->
 
         ok = rabbit_boot_steps:run_boot_steps([rabbit | Plugins]),
         run_postlaunch_phase(Plugins),
+        rabbit_boot_state:set(core_started),
         {ok, SupPid}
     catch
         throw:{error, _} = Error ->
diff --git a/deps/rabbit/src/rabbit_direct.erl b/deps/rabbit/src/rabbit_direct.erl
index 3fc2d75908..0506e1bf6c 100644
--- a/deps/rabbit/src/rabbit_direct.erl
+++ b/deps/rabbit/src/rabbit_direct.erl
@@ -75,7 +75,7 @@ auth_fun({Username, Password}, VHost, ExtraAuthProps) ->
 connect(Creds, VHost, Protocol, Pid, Infos) ->
     ExtraAuthProps = extract_extra_auth_props(Creds, VHost, Pid, Infos),
     AuthFun = auth_fun(Creds, VHost, ExtraAuthProps),
-    case rabbit:is_running() of
+    case rabbit_boot_state:has_reached_and_is_active(core_started) of
         true  ->
             case whereis(rabbit_direct_client_sup) of
                 undefined ->

What do you think?

[dumbbell]

There are still some errors reported by CI. I will continue to study them tomorrow.

[dumbbell]

I could only reproduce one failure in rabbitmq_stomp and it went away after I updated my local copy of osiris. So I rebased the branch and force-pushed, just in case the previous builds were running with inconsistencies between RabbitMQ and osiris' master branch.

[lukebakken]

Thanks @dumbbell I'm going to wrap up testing this today.

[lukebakken]

@dumbbell @noxdafox I have confirmed that this PR #2656 fixes the issue that the changes in noxdafox/rabbitmq-message-deduplication#62 address. Here's how I tested:

• I checked out the https://github.com/noxdafox/rabbitmq-message-deduplication repo as a sub-directory of deps/ in my rabbitmq/rabbitmq-server clone.
• I reset the commit to noxdafox/rabbitmq-message-deduplication@c3e302c, i.e. just before the changes in RabbitMQ w/ dedup plugin enabled appears to hang on start_app after 'reset' noxdafox/rabbitmq-message-deduplication#62
• Built the plugin cd deps/rabbitmq_message_deduplication && make dist
• Started the broker this way:

  make PLUGINS='rabbitmq_management rabbitmq_top rabbitmq_message_deduplication' LOG=debug run-broker
  

• Declared a queue with the x-message-deduplication argument. Published two messages using the same de-duplication header, and confirmed that the second message is rejected.
• Restarted RabbitMQ
• Re-did the publish sequence and saw the same behavior. I'm assuming that if the queue did not start correctly I would not have seen the second message be rejected.

[michaelklishin]

I can confirm that this does not negatively affect rabbitmq_consistent_hash_exchange or rabbitmq_delayed_message_exchange, two stateful exchange plugins, including when nodes restart or definitions containing entities of types provided by the plugins are imported.

[michaelklishin]

Backported to v3.8.x.

[noxdafox]

@lukebakken I confirm your test approach to be correct. Thanks!