[kernel] Reduce risk of dead lock when terminating logger_sup

8 files changed, 144 insertions, 24 deletions

diff --git a/lib/kernel/src/Makefile b/lib/kernel/src/Makefile
index c595c25341..57f17defc8 100644
--- a/lib/kernel/src/Makefile
+++ b/lib/kernel/src/Makefile
@@ -112,7 +112,8 @@ MODULES = \
         logger \
         logger_backend \
         logger_config \
-        logger_std_h \
+	logger_handler_watcher \
+	logger_std_h \
 	logger_disk_log_h \
 	logger_h_common \
 	logger_filters \
diff --git a/lib/kernel/src/error_logger.erl b/lib/kernel/src/error_logger.erl
index a7e7f19167..ad8c937882 100644
--- a/lib/kernel/src/error_logger.erl
+++ b/lib/kernel/src/error_logger.erl
@@ -74,8 +74,8 @@ start() ->
                   type => worker,
                   modules => dynamic},
             case supervisor:start_child(logger_sup, ErrorLogger) of
-                {ok,_} ->
-                    ok;
+                {ok,Pid} ->
+                    ok = logger_handler_watcher:register_handler(?MODULE,Pid);
                 Error ->
                     Error
             end;
@@ -95,9 +95,14 @@ start_link() ->
 %%% Stop the event manager
 -spec stop() -> ok.
 stop() ->
-    _ = supervisor:terminate_child(logger_sup,?MODULE),
-    _ = supervisor:delete_child(logger_sup,?MODULE),
-    ok.
+    case whereis(?MODULE) of
+        undefined ->
+            ok;
+        _Pid ->
+            _ = gen_event:stop(?MODULE,{shutdown,stopped},infinity),
+            _ = supervisor:delete_child(logger_sup,?MODULE),
+            ok
+    end.
 
 %%%-----------------------------------------------------------------
 %%% Callbacks for logger
diff --git a/lib/kernel/src/kernel.app.src b/lib/kernel/src/kernel.app.src
index 47dd7c03d5..4933eae76f 100644
--- a/lib/kernel/src/kernel.app.src
+++ b/lib/kernel/src/kernel.app.src
@@ -67,6 +67,7 @@
              logger_filters,
              logger_formatter,
              logger_h_common,
+             logger_handler_watcher,
              logger_server,
              logger_simple_h,
              logger_std_h,
@@ -129,6 +130,7 @@
 		kernel_refc,
 		kernel_sup,
                 logger,
+                logger_handler_watcher,
                 logger_sup,
 		net_kernel,
 		net_sup,
diff --git a/lib/kernel/src/logger_disk_log_h.erl b/lib/kernel/src/logger_disk_log_h.erl
index 0a72654e11..e56531c3cb 100644
--- a/lib/kernel/src/logger_disk_log_h.erl
+++ b/lib/kernel/src/logger_disk_log_h.erl
@@ -488,7 +488,8 @@ start(Name, Config, HandlerState) ->
           type     => worker,
           modules  => [?MODULE]},
     case supervisor:start_child(logger_sup, LoggerDLH) of
-        {ok,_Pid,Config1} ->
+        {ok,Pid,Config1} ->
+            ok = logger_handler_watcher:register_handler(Name,Pid),
             {ok,Config1};
         Error ->
             Error
@@ -506,8 +507,11 @@ stop(Name) ->
             %% system termination in order to avoid circular attempts
             %% at removing the handler (implying deadlocks and
             %% timeouts).
+            %% And we don't need to do supervisor:delete_child, since
+            %% the restart type is temporary, which means that the
+            %% child specification is automatically removed from the
+            %% supervisor when the process dies.
             _ = gen_server:call(Pid, stop),
-            _ = supervisor:delete_child(logger_sup, Name),
             ok
     end.
 
diff --git a/lib/kernel/src/logger_h_common.erl b/lib/kernel/src/logger_h_common.erl
index a40345dddc..854e5479b9 100644
--- a/lib/kernel/src/logger_h_common.erl
+++ b/lib/kernel/src/logger_h_common.erl
@@ -309,19 +309,6 @@ stop_or_restart(Name, {shutdown,Reason={overloaded,_Name,_QLen,_Mem}},
         end,
     spawn(RemoveAndRestart),
     ok;
-
-stop_or_restart(Name, shutdown, _State) ->
-    %% Probably terminated by supervisor. Remove the handler to avoid
-    %% error printouts due to failing handler.
-    _ = case logger:get_handler_config(Name) of
-            {ok,_} ->
-                %% Spawning to avoid deadlock
-                spawn(logger,remove_handler,[Name]);
-            _ ->
-                ok
-        end,
-    ok;
-
 stop_or_restart(_Name, _Reason, _State) ->
     ok.
 
diff --git a/lib/kernel/src/logger_handler_watcher.erl b/lib/kernel/src/logger_handler_watcher.erl
new file mode 100644
index 0000000000..b75c74c643
--- /dev/null
+++ b/lib/kernel/src/logger_handler_watcher.erl
@@ -0,0 +1,113 @@
+%%
+%% %CopyrightBegin%
+%%
+%% Copyright Ericsson AB 2018. All Rights Reserved.
+%%
+%% Licensed under the Apache License, Version 2.0 (the "License");
+%% you may not use this file except in compliance with the License.
+%% You may obtain a copy of the License at
+%%
+%%     http://www.apache.org/licenses/LICENSE-2.0
+%%
+%% Unless required by applicable law or agreed to in writing, software
+%% distributed under the License is distributed on an "AS IS" BASIS,
+%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%
+%% %CopyrightEnd%
+%%
+-module(logger_handler_watcher).
+
+-behaviour(gen_server).
+
+%% API
+-export([start_link/0]).
+-export([register_handler/2]).
+
+%% gen_server callbacks
+-export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2]).
+
+-define(SERVER, ?MODULE).
+
+-record(state, {handlers}).
+
+%%%===================================================================
+%%% API
+%%%===================================================================
+
+-spec start_link() -> {ok, Pid :: pid()} |
+                      {error, Error :: {already_started, pid()}} |
+                      {error, Error :: term()} |
+                      ignore.
+start_link() ->
+    gen_server:start_link({local, ?SERVER}, ?MODULE, [], []).
+
+-spec register_handler(Id::logger:handler_id(),Pid::pid()) -> ok.
+register_handler(Id,Pid) ->
+    gen_server:call(?SERVER,{register,Id,Pid}).
+
+%%%===================================================================
+%%% gen_server callbacks
+%%%===================================================================
+
+-spec init(Args :: term()) -> {ok, State :: term()} |
+                              {ok, State :: term(), Timeout :: timeout()} |
+                              {ok, State :: term(), hibernate} |
+                              {stop, Reason :: term()} |
+                              ignore.
+init([]) ->
+    process_flag(trap_exit, true),
+    {ok, #state{handlers=[]}}.
+
+-spec handle_call(Request :: term(), From :: {pid(), term()}, State :: term()) ->
+                         {reply, Reply :: term(), NewState :: term()} |
+                         {reply, Reply :: term(), NewState :: term(), Timeout :: timeout()} |
+                         {reply, Reply :: term(), NewState :: term(), hibernate} |
+                         {noreply, NewState :: term()} |
+                         {noreply, NewState :: term(), Timeout :: timeout()} |
+                         {noreply, NewState :: term(), hibernate} |
+                         {stop, Reason :: term(), Reply :: term(), NewState :: term()} |
+                         {stop, Reason :: term(), NewState :: term()}.
+handle_call({register,Id,Pid}, _From, #state{handlers=Hs}=State) ->
+    Ref = erlang:monitor(process,Pid),
+    Hs1 = lists:keystore(Id,1,Hs,{Id,Ref}),
+    {reply, ok, State#state{handlers=Hs1}}.
+
+-spec handle_cast(Request :: term(), State :: term()) ->
+                         {noreply, NewState :: term()} |
+                         {noreply, NewState :: term(), Timeout :: timeout()} |
+                         {noreply, NewState :: term(), hibernate} |
+                         {stop, Reason :: term(), NewState :: term()}.
+handle_cast(_Request, State) ->
+    {noreply, State}.
+
+-spec handle_info(Info :: timeout() | term(), State :: term()) ->
+                         {noreply, NewState :: term()} |
+                         {noreply, NewState :: term(), Timeout :: timeout()} |
+                         {noreply, NewState :: term(), hibernate} |
+                         {stop, Reason :: normal | term(), NewState :: term()}.
+handle_info({'DOWN',Ref,process,_,shutdown}, #state{handlers=Hs}=State) ->
+    case lists:keytake(Ref,2,Hs) of
+        {value,{Id,Ref},Hs1} ->
+            %% Probably terminated by supervisor. Remove the handler to avoid
+            %% error printouts due to failing handler.
+            _ = case logger:get_handler_config(Id) of
+                    {ok,_} ->
+                        logger:remove_handler(Id);
+                    _ ->
+                        ok
+                end,
+            {noreply,State#state{handlers=Hs1}};
+        false ->
+            {noreply, State}
+    end;
+handle_info({'DOWN',Ref,process,_,_OtherReason}, #state{handlers=Hs}=State) ->
+    {noreply,State#state{handlers=lists:keydelete(Ref,2,Hs)}};
+handle_info(_Other,State) ->
+    {noreply,State}.
+
+-spec terminate(Reason :: normal | shutdown | {shutdown, term()} | term(),
+                State :: term()) -> any().
+terminate(_Reason, _State) ->
+    ok.
diff --git a/lib/kernel/src/logger_std_h.erl b/lib/kernel/src/logger_std_h.erl
index 480fafd6d8..9a2a1443b3 100644
--- a/lib/kernel/src/logger_std_h.erl
+++ b/lib/kernel/src/logger_std_h.erl
@@ -467,7 +467,8 @@ start(Name, Config, HandlerState) ->
           type     => worker,
           modules  => [?MODULE]},
     case supervisor:start_child(logger_sup, LoggerStdH) of
-        {ok,_Pid,Config1} ->
+        {ok,Pid,Config1} ->
+            ok = logger_handler_watcher:register_handler(Name,Pid),
             {ok,Config1};
         Error ->
             Error
@@ -485,8 +486,11 @@ stop(Name) ->
             %% system termination in order to avoid circular attempts
             %% at removing the handler (implying deadlocks and
             %% timeouts).
+            %% And we don't need to do supervisor:delete_child, since
+            %% the restart type is temporary, which means that the
+            %% child specification is automatically removed from the
+            %% supervisor when the process dies.
             _ = gen_server:call(Pid, stop),
-            _ = supervisor:delete_child(logger_sup, Name),
             ok
     end.
 
diff --git a/lib/kernel/src/logger_sup.erl b/lib/kernel/src/logger_sup.erl
index dcdcdad0bd..3d6f482e20 100644
--- a/lib/kernel/src/logger_sup.erl
+++ b/lib/kernel/src/logger_sup.erl
@@ -46,7 +46,11 @@ init([]) ->
                  intensity => 1,
                  period => 5},
 
-    {ok, {SupFlags, []}}.
+    Watcher = #{id => logger_handler_watcher,
+                start => {logger_handler_watcher, start_link, []},
+                shutdown => brutal_kill},
+
+    {ok, {SupFlags, [Watcher]}}.
 
 %%%===================================================================
 %%% Internal functions