Merge branch 'lukas/kernel/fix-application-shutdown-deadlock/OTP-19078' into maint-26

* lukas/kernel/fix-application-shutdown-deadlock/OTP-19078:
  kernel: Fix group to not crash if group_history fails
  kernel: Fix AC to respond to calls while terminating

Author: Erlang/OTP

lib/kernel/src/application_controller.erl

@@ -209,10 +209,10 @@ start(KernelApp) ->
 %% Returns: ok | {error, Reason}
 %%-----------------------------------------------------------------
 load_application(Application) ->
-    gen_server:call(?AC, {load_application, Application}, infinity).
+    call({load_application, Application}, infinity).
 
 unload_application(AppName) ->
-    gen_server:call(?AC, {unload_application, AppName}, infinity).
+    call({unload_application, AppName}, infinity).
 
 %%-----------------------------------------------------------------
 %% Func: start_application/2
@@ -236,7 +236,7 @@ unload_application(AppName) ->
 %% Returns: ok | {error, Reason}
 %%-----------------------------------------------------------------
 start_application(AppName, RestartType) ->
-    gen_server:call(?AC, {start_application, AppName, RestartType}, infinity).
+    call({start_application, AppName, RestartType}, infinity).
 
 start_application_request(AppName, RestartType) ->
     gen_server:send_request(?AC, {start_application, AppName, RestartType}).
@@ -250,7 +250,7 @@ start_application_request(AppName, RestartType) ->
 %% Returns: boolean
 %%-----------------------------------------------------------------
 is_running(AppName) when is_atom(AppName) ->
-    gen_server:call(?AC, {is_running, AppName}, infinity).
+    call({is_running, AppName}, infinity).
 
 %%-----------------------------------------------------------------
 %% Func: start_boot_application/2
@@ -263,9 +263,9 @@ start_boot_application(Application, RestartType) ->
     case {application:load(Application), RestartType} of
 	{ok, _} ->
 	    AppName = get_appl_name(Application),
-	    gen_server:call(?AC, {start_application, AppName, RestartType}, infinity);
+	    call({start_application, AppName, RestartType}, infinity);
 	{{error, {already_loaded, AppName}}, _} ->
-	    gen_server:call(?AC, {start_application, AppName, RestartType}, infinity);
+	    call({start_application, AppName, RestartType}, infinity);
 	{{error,{bad_environment_value,Env}}, permanent} ->
 	    Txt = io_lib:format("Bad environment variable: ~tp  Application: ~p",
 				[Env, Application]),
@@ -275,15 +275,15 @@ start_boot_application(Application, RestartType) ->
     end.
 
 stop_application(AppName) ->
-    gen_server:call(?AC, {stop_application, AppName}, infinity).
+    call({stop_application, AppName}, infinity).
 
 %%-----------------------------------------------------------------
 %% Returns: [{Name, Descr, Vsn}]
 %%-----------------------------------------------------------------
 which_applications() ->
-    gen_server:call(?AC, which_applications).    
+    call(which_applications).
 which_applications(Timeout) ->
-    gen_server:call(?AC, which_applications, Timeout).
+    call(which_applications, Timeout).
 
 loaded_applications() ->
     ets:select(ac_tab,
@@ -295,10 +295,10 @@ loaded_applications() ->
 
 %% Returns some debug info
 info() ->
-    gen_server:call(?AC, info).    
+    call(info).
 
 control_application(AppName) ->
-    gen_server:call(?AC, {control_application, AppName}, infinity).
+    call({control_application, AppName}, infinity).
 
 %%-----------------------------------------------------------------
 %% Func: change_application_data/2
@@ -323,21 +323,14 @@ control_application(AppName) ->
 %%          some applicatation may have got new config data.
 %%-----------------------------------------------------------------
 change_application_data(Applications, Config) ->
-    gen_server:call(?AC, 
-		    {change_application_data, Applications, Config},
-		    infinity).
+    call({change_application_data, Applications, Config},infinity).
 
 prep_config_change() ->
-    gen_server:call(?AC, 
-		    prep_config_change,
-		    infinity).
+    call(prep_config_change, infinity).
 
 
 config_change(EnvPrev) ->
-    gen_server:call(?AC, 
-		    {config_change, EnvPrev},
-		    infinity).
-
+    call({config_change, EnvPrev},infinity).
 
 
 get_pid_env(Master, Key) ->
@@ -439,7 +432,7 @@ get_all_key(AppName) ->
 start_type(Master) ->
     case ets:match(ac_tab, {{application_master, '$1'}, Master}) of
 	[[AppName]] -> 
-	    gen_server:call(?AC, {start_type, AppName}, infinity);
+	    call({start_type, AppName}, infinity);
 	_X -> 
 	    undefined
     end.
@@ -475,31 +468,44 @@ get_application_module(_Module, []) ->
     undefined.
 
 permit_application(ApplName, Flag) ->
-    gen_server:call(?AC, 
-		    {permit_application, ApplName, Flag},
-		    infinity).
+    call({permit_application, ApplName, Flag},infinity).
 
 set_env(Config, Opts) ->
     case check_conf_data(Config) of
 	ok ->
 	    Timeout = proplists:get_value(timeout, Opts, 5000),
-	    gen_server:call(?AC, {set_env, Config, Opts}, Timeout);
+	    call({set_env, Config, Opts}, Timeout);
 
 	{error, _} = Error ->
 	    Error
     end.
 
 set_env(AppName, Key, Val) ->
-    gen_server:call(?AC, {set_env, AppName, Key, Val, []}).
+    call({set_env, AppName, Key, Val, []}).
 set_env(AppName, Key, Val, Opts) ->
     Timeout = proplists:get_value(timeout, Opts, 5000),
-    gen_server:call(?AC, {set_env, AppName, Key, Val, Opts}, Timeout).
+    call({set_env, AppName, Key, Val, Opts}, Timeout).
 
 unset_env(AppName, Key) ->
-    gen_server:call(?AC, {unset_env, AppName, Key, []}).
+    call({unset_env, AppName, Key, []}).
 unset_env(AppName, Key, Opts) ->
     Timeout = proplists:get_value(timeout, Opts, 5000),
-    gen_server:call(?AC, {unset_env, AppName, Key, Opts}, Timeout).
+    call({unset_env, AppName, Key, Opts}, Timeout).
+
+call(Cmd) ->
+    case gen_server:call(?AC, Cmd) of
+        {error, terminating} ->
+            exit(terminating);
+        Res ->
+            Res
+    end.
+call(Cmd, Timeout) ->
+    case gen_server:call(?AC, Cmd, Timeout) of
+        {error, terminating} ->
+            exit(terminating);
+        Res ->
+            Res
+    end.
 
 %%%-----------------------------------------------------------------
 %%% call-back functions from gen_server
@@ -1233,14 +1239,21 @@ terminate(Reason, S) ->
 			%% Proc died before link
 			{'EXIT', Id, _} -> ok
 		    after 0 ->
-			    receive
-				{'DOWN', Ref, process, Id, _} -> ok
-			    after ShutdownTimeout ->
-				    exit(Id, kill),
-				    receive
-					{'DOWN', Ref, process, Id, _} -> ok
-				    end
-			    end
+                            (fun F() ->
+                                     receive
+                                         {'DOWN', Ref, process, Id, _} -> ok;
+                                         %% We need to handle any gen_server:call here
+                                         %% and reply to them so that they don't deadlock
+                                         {'$gen_call', From, _Msg} ->
+                                             gen_server:reply(From, {error, terminating}),
+                                             F()
+                                     after ShutdownTimeout ->
+                                             exit(Id, kill),
+                                             receive
+                                                 {'DOWN', Ref, process, Id, _} -> ok
+                                             end
+                                     end
+                             end)()
 		    end;
 	       (_) -> ok
 	    end,

lib/kernel/src/group.erl

@@ -19,6 +19,8 @@
 %%
 -module(group).
 
+-include_lib("kernel/include/logger.hrl").
+
 %% A group leader process for user io.
 %% This process receives input data from user_drv in this format
 %%   {Drv,{data,unicode:charlist()}}
@@ -1005,7 +1007,12 @@ save_line_buffer("\n", Lines) ->
 save_line_buffer(Line, [Line|_Lines]=Lines) ->
     save_line_buffer(Lines);
 save_line_buffer(Line, Lines) ->
-    group_history:add(Line),
+    try
+        group_history:add(Line)
+    catch E:R:ST ->
+            ?LOG_ERROR(#{ msg => "Failed to write to shell history",
+                          error => {E, R, ST} })
+    end,
     save_line_buffer([Line|Lines]).
 
 save_line_buffer(Lines) ->

lib/kernel/src/group_history.erl

@@ -374,7 +374,10 @@ find_path() ->
 to_drop() ->
     case application:get_env(kernel, shell_history_drop) of
         undefined ->
-            application:set_env(kernel, shell_history_drop, ?DEFAULT_DROP),
+            %% The AC might be overloaded/not responding and
+            %% we want the shell to be as responsive as possible
+            %% so we set a short timeout
+            application:set_env(kernel, shell_history_drop, ?DEFAULT_DROP, [{timeout, 10}]),
             ?DEFAULT_DROP;
         {ok, V} when is_list(V) -> [Ln++"\n" || Ln <- V];
         {ok, _} -> ?DEFAULT_DROP

lib/kernel/src/kernel.app.src

@@ -157,7 +157,8 @@
          {net_tickintensity, 4},
          {net_ticktime, 60},
          {prevent_overlapping_partitions, true},
-         {shell_docs_ansi,auto}
+         {shell_docs_ansi,auto},
+         {shell_history_drop,[]}
         ]},
   {mod, {kernel, []}},
   {runtime_dependencies, ["erts-14.0", "stdlib-5.0",

lib/kernel/test/application_SUITE.erl

@@ -20,6 +20,7 @@
 -module(application_SUITE).
 
 -include_lib("common_test/include/ct.hrl").
+-include_lib("stdlib/include/assert.hrl").
 
 -export([all/0, suite/0,groups/0,init_per_suite/1, end_per_suite/1, 
 	 init_per_group/2,end_per_group/2
@@ -38,7 +39,8 @@
 -export([config_change/1, persistent_env/1, invalid_app_file/1,
 	 distr_changed_tc1/1, distr_changed_tc2/1,
 	 ensure_started/1, ensure_all_started/1,
-	 shutdown_func/1, do_shutdown/1, shutdown_timeout/1, shutdown_deadlock/1,
+	 shutdown_func/1, do_shutdown/1, shutdown_timeout/1,
+         shutdown_application_call/1,shutdown_deadlock/1,
          config_relative_paths/1, handle_many_config_files/1,
          format_log_1/1, format_log_2/1,
          configfd_bash/1, configfd_port_program/1]).
@@ -60,7 +62,7 @@ all() ->
      permit_false_start_dist, get_key, get_env, ensure_all_started,
      set_env, set_env_persistent, set_env_errors, get_supervisor,
      {group, distr_changed}, config_change, shutdown_func, shutdown_timeout,
-     shutdown_deadlock, config_relative_paths, optional_applications,
+     shutdown_application_call, shutdown_deadlock, config_relative_paths, optional_applications,
      persistent_env, handle_many_config_files, format_log_1, format_log_2,
      configfd_bash, configfd_port_program, invalid_app_file].
 
@@ -2576,6 +2578,60 @@ shutdown_timeout(Config) when is_list(Config) ->
     end,
     ok.
 
+%%%-----------------------------------------------------------------
+%%% Test that we do not cause a deadlock if we call
+%%% application:set_env or application:ensure_started
+%%% when terminating
+%%%-----------------------------------------------------------------
+shutdown_application_call(Config) when is_list(Config) ->
+    Tester = self(),
+    shutdown_application_call(
+      fun() ->
+              Tester ! {Tester,
+                        catch application:set_env(
+                                deadlock, a, b, [{timeout, infinity},
+                                                 {persistent, true}])}
+      end, Config),
+    receive
+        {Tester, M} ->
+            ?assertMatch({'EXIT',terminating}, M)
+    after 1000 ->
+            ct:fail("timeout 1 sec: no crash message found")
+    end,
+
+    shutdown_application_call(
+      fun() ->
+              Tester ! {Tester, catch application:ensure_started(runtime_tools)}
+      end, Config),
+    receive
+        {Tester, M2}  ->
+            ?assertMatch({'EXIT',terminating}, M2)
+    after 1000 ->
+            ct:fail("timeout 1 sec: no crash message found")
+    end.
+
+shutdown_application_call(Fun, Config) ->
+
+    DataDir = proplists:get_value(data_dir,Config),
+    {ok,Cp1} = start_node(?MODULE_STRING++"_"++atom_to_list(?FUNCTION_NAME)),
+    wait_for_ready_net(),
+    rpc:call(Cp1, code, add_path, [filename:join([DataDir,deadlock])]),
+    rpc:call(Cp1, code, add_path, [filename:dirname(code:which(?MODULE))]),
+    ok = rpc:call(Cp1, application, start, [sasl]),
+
+    ok = rpc:call(Cp1, application, start, [deadlock]),
+    rpc:call(Cp1, application, set_env, [deadlock, fail_stop, Fun]),
+
+    ok = net_kernel:monitor_nodes(true),
+    _ = rpc:call(Cp1, init, stop, []),
+    receive
+	{nodedown,Cp1} ->
+            ok
+    after 10000 ->
+	    ct:fail("timeout 10 sec: node termination hangs")
+    end,
+    ok.
+
 %%%-----------------------------------------------------------------
 %%% Provokes a (previous) application shutdown deadlock
 %%%-----------------------------------------------------------------

lib/kernel/test/application_SUITE_data/deadlock/deadlock.erl

@@ -54,7 +54,9 @@ handle_info(_Msg, State) ->
 terminate(_Reason, _State) ->
     case application:get_env(deadlock, fail_stop) of
         {ok, false} -> ok;
-        {ok, Tester}  ->
+        {ok, Fun} when is_function(Fun, 0) ->
+            Fun();
+        {ok, Tester} when is_pid(Tester) ->
 	    Tester ! {deadlock, self()},
 	    io:format("~p: Waiting in terminate (~p)~n",[?MODULE,Tester]),
 	    receive continue -> ok end

lib/sasl/test/release_handler_SUITE.erl

@@ -2388,7 +2388,10 @@ wait_nodes_up(Nodes, Tag, Apps, N) ->
 	fun(NodeInfo={Node,OldInitPid}, A) ->
 		case rpc:call(Node, application, which_applications, []) of
 		    {badrpc, nodedown} ->
-			test_server:format( "  ~p = {badarg, nodedown}",[Node]),
+			test_server:format( "  ~p = {badrpc, nodedown}",[Node]),
+			[NodeInfo | A];
+                    {badrpc, {'EXIT',terminating}} ->
+			test_server:format( "  ~p = {badrpc, {'EXIT',terminating}}",[Node]),
 			[NodeInfo | A];
 		    List when is_list(List)->
 			test_server:format( "  ~p = [~p]",[Node, List]),
@@ -2406,7 +2409,7 @@ wait_nodes_up(Nodes, Tag, Apps, N) ->
 			    false ->
 				[NodeInfo | A]
 			end
-		end
+                end
 	end,
     Pang = lists:foldl(Fun,[],Nodes),
     case Pang of