rabbit_misc:shutdown_supervisor/1: log on timeout, don't crash the caller rabbitmq#16598

Author: michaelklishin

deps/rabbit/src/rabbit_channel_sup.erl

@@ -38,8 +38,6 @@
          rabbit_types:user(), rabbit_types:vhost(),
          rabbit_framing:amqp_table(), pid()}.
 
--define(FAIR_WAIT, 70000).
-
 %%----------------------------------------------------------------------------
 
 -spec start_link(start_link_args()) ->
@@ -63,6 +61,8 @@ start_link({tcp, Sock, Channel, FrameMax, ReaderPid, ConnName, User,
                   shutdown => ?FAIR_WAIT,
                   type => worker,
                   modules => [rabbit_channel]},
+    %% For potential backports to 4.1: this call site will need adjustment compared to 4.2 and newer series.
+    %% Thread `Protocol` through `start_link_args/0` and `rabbit_command_assembler:init/1`.
     case supervisor:start_child(SupPid, ChildSpec) of
         {ok, ChannelPid} ->
             {ok, AState} = rabbit_command_assembler:init(),

deps/rabbit/test/channel_interceptor_SUITE.erl

@@ -277,9 +277,9 @@ conflicting_interceptors_close_direct_connections_gracefully1(Config) ->
         {ok, Conn} = amqp_connection:start(Params),
         ?assert(is_pid(Conn)),
         ?assert(is_process_alive(Conn)),
-        %% open_channel must return {error, _} gracefully, not crash.
+        %% `open_channel` must return `{error, _}` gracefully, not crash.
         %% If the connection process crashes (e.g. badmatch in
-        %% amqp_channels_manager), this call will throw and fail the test.
+        %% `amqp_channels_manager`), this call will throw and fail the test.
         Result = amqp_connection:open_channel(Conn),
         ?assertMatch({error, _}, Result),
         ?assert(is_process_alive(Conn)),

deps/rabbit_common/include/rabbit.hrl

@@ -226,6 +226,7 @@
 -define(EMPTY_FRAME_SIZE, 8).
 
 -define(MAX_WAIT, 16#ffffffff).
+-define(FAIR_WAIT, 70_000).
 -define(SUPERVISOR_WAIT,
         rabbit_misc:get_env(rabbit, supervisor_shutdown_timeout, infinity)).
 -define(WORKER_WAIT,

deps/rabbit_common/include/rabbit_misc.hrl

@@ -7,4 +7,3 @@
 
 -define(RPC_TIMEOUT, 15_000).
 -define(RPC_INFINITE_TIMEOUT, infinity).
--define(DEFAULT_TIMEOUT, 5_000).

deps/rabbit_common/src/rabbit_misc.erl

@@ -1436,14 +1436,20 @@ find_child(Supervisor, Name) ->
     [Pid || {Name1, Pid, _Type, _Modules} <- supervisor:which_children(Supervisor),
             Name1 =:= Name].
 
+%% Must be called by the process that holds the link to SupPid.
+-spec shutdown_supervisor(pid()) -> ok.
 shutdown_supervisor(SupPid) when is_pid(SupPid) ->
     MRef = erlang:monitor(process, SupPid),
     unlink(SupPid),
     exit(SupPid, shutdown),
     receive
         {'DOWN', MRef, process, SupPid, _} -> ok
-    after ?DEFAULT_TIMEOUT ->
-        exit({shutdown_supervisor_timeout, SupPid})
+    after ?FAIR_WAIT ->
+        erlang:demonitor(MRef, [flush]),
+        ?LOG_WARNING(
+            "rabbit_misc:shutdown_supervisor/1 timed out waiting for ~tp to terminate",
+            [SupPid]),
+        ok
     end.
 
 %% -------------------------------------------------------------------------