aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorSiri Hansen <[email protected]>2018-07-06 15:27:42 +0200
committerSiri Hansen <[email protected]>2018-07-13 12:20:26 +0200
commit0a4834e59f2aac7b76331410bcee641d375d8ec0 (patch)
treea81ce74a20a33777f5f5c2d45cb908b6bf560b74
parent4ae5136ccb68bed0e72e1f45d34d1a8b44594590 (diff)
downloadotp-0a4834e59f2aac7b76331410bcee641d375d8ec0.tar.gz
otp-0a4834e59f2aac7b76331410bcee641d375d8ec0.tar.bz2
otp-0a4834e59f2aac7b76331410bcee641d375d8ec0.zip
[kernel] Reduce risk of dead lock when terminating logger_sup
-rw-r--r--lib/kernel/src/Makefile3
-rw-r--r--lib/kernel/src/error_logger.erl15
-rw-r--r--lib/kernel/src/kernel.app.src2
-rw-r--r--lib/kernel/src/logger_disk_log_h.erl8
-rw-r--r--lib/kernel/src/logger_h_common.erl13
-rw-r--r--lib/kernel/src/logger_handler_watcher.erl113
-rw-r--r--lib/kernel/src/logger_std_h.erl8
-rw-r--r--lib/kernel/src/logger_sup.erl6
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