From b74a978af2253773a2d74876d4f4abccde25a673 Mon Sep 17 00:00:00 2001
From: Rory Byrne <rory@nybek.com>
Date: Thu, 28 May 2015 15:48:42 +0100
Subject: Fix supervisor:get_childspec/2 for simple_one_for_one

A bug in supervisor:get_childspec/2 results in
{error, simple_one_for_one} being returned on every call when the
supervisor strategy is simple_one_for_one.

This commit includes a small refactoring which brings together the
two 'start_child' clauses to make the code easier to follow.
---
 lib/stdlib/src/supervisor.erl        | 27 +++++++++++++++------------
 lib/stdlib/test/supervisor_SUITE.erl | 29 +++++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 14 deletions(-)

(limited to 'lib/stdlib')

diff --git a/lib/stdlib/src/supervisor.erl b/lib/stdlib/src/supervisor.erl
index 1d7396adee..ac51bb3b79 100644
--- a/lib/stdlib/src/supervisor.erl
+++ b/lib/stdlib/src/supervisor.erl
@@ -393,6 +393,15 @@ handle_call({start_child, EArgs}, _From, State) when ?is_simple(State) ->
 	    {reply, What, State}
     end;
 
+handle_call({start_child, ChildSpec}, _From, State) ->
+    case check_childspec(ChildSpec) of
+	{ok, Child} ->
+	    {Resp, NState} = handle_start_child(Child, State),
+	    {reply, Resp, NState};
+	What ->
+	    {reply, {error, What}, State}
+    end;
+
 %% terminate_child for simple_one_for_one can only be done with pid
 handle_call({terminate_child, Name}, _From, State) when not is_pid(Name),
 							?is_simple(State) ->
@@ -411,20 +420,10 @@ handle_call({terminate_child, Name}, _From, State) ->
 	    {reply, {error, not_found}, State}
     end;
 
-%%% The requests delete_child and restart_child are invalid for
-%%% simple_one_for_one supervisors.
-handle_call({_Req, _Data}, _From, State) when ?is_simple(State) ->
+%% restart_child request is invalid for simple_one_for_one supervisors
+handle_call({restart_child, _Name}, _From, State) when ?is_simple(State) ->
     {reply, {error, simple_one_for_one}, State};
 
-handle_call({start_child, ChildSpec}, _From, State) ->
-    case check_childspec(ChildSpec) of
-	{ok, Child} ->
-	    {Resp, NState} = handle_start_child(Child, State),
-	    {reply, Resp, NState};
-	What ->
-	    {reply, {error, What}, State}
-    end;
-
 handle_call({restart_child, Name}, _From, State) ->
     case get_child(Name, State) of
 	{value, Child} when Child#child.pid =:= undefined ->
@@ -446,6 +445,10 @@ handle_call({restart_child, Name}, _From, State) ->
 	    {reply, {error, not_found}, State}
     end;
 
+%% delete_child request is invalid for simple_one_for_one supervisors
+handle_call({delete_child, _Name}, _From, State) when ?is_simple(State) ->
+    {reply, {error, simple_one_for_one}, State};
+
 handle_call({delete_child, Name}, _From, State) ->
     case get_child(Name, State) of
 	{value, Child} when Child#child.pid =:= undefined ->
diff --git a/lib/stdlib/test/supervisor_SUITE.erl b/lib/stdlib/test/supervisor_SUITE.erl
index 015b09f35e..31d4b44f30 100644
--- a/lib/stdlib/test/supervisor_SUITE.erl
+++ b/lib/stdlib/test/supervisor_SUITE.erl
@@ -39,7 +39,8 @@
 	  sup_start_ignore_temporary_child_start_child_simple/1,
           sup_start_ignore_permanent_child_start_child_simple/1,
 	  sup_start_error_return/1, sup_start_fail/1,
-	  sup_start_map/1, sup_start_map_faulty_specs/1,
+	  sup_start_map/1, sup_start_map_simple/1,
+	  sup_start_map_faulty_specs/1,
 	  sup_stop_infinity/1, sup_stop_timeout/1, sup_stop_brutal_kill/1,
 	  child_adm/1, child_adm_simple/1, child_specs/1, extra_return/1,
 	  sup_flags/1]).
@@ -103,7 +104,7 @@ groups() ->
        sup_start_ignore_permanent_child_start_child_simple,
        sup_start_error_return, sup_start_fail]},
      {sup_start_map, [],
-      [sup_start_map, sup_start_map_faulty_specs]},
+      [sup_start_map, sup_start_map_simple, sup_start_map_faulty_specs]},
      {sup_stop, [],
       [sup_stop_infinity, sup_stop_timeout,
        sup_stop_brutal_kill]},
@@ -322,6 +323,30 @@ sup_start_map(Config) when is_list(Config) ->
     {error,not_found} = supervisor:get_childspec(Pid, child4),
     terminate(Pid, shutdown).
 
+%%-------------------------------------------------------------------------
+%% Tests that the supervisor process starts correctly with map
+%% startspec, and that the full childspec can be read when using
+%% simple_one_for_one strategy.
+sup_start_map_simple(Config) when is_list(Config) ->
+    process_flag(trap_exit, true),
+    SupFlags = #{strategy=>simple_one_for_one},
+    ChildSpec = #{id=>undefined,
+		  start=>{supervisor_1, start_child, []},
+		  restart=>temporary},
+    {ok, Pid} = start_link({ok, {SupFlags, [ChildSpec]}}),
+
+    {ok, Child1} = supervisor:start_child(Pid, []),
+    {ok, Child2} = supervisor:start_child(Pid, []),
+    {ok, Child3} = supervisor:start_child(Pid, []),
+
+    Spec = ChildSpec#{type=>worker, shutdown=>5000, modules=>[supervisor_1]},
+
+    {ok, Spec} = supervisor:get_childspec(Pid, Child1),
+    {ok, Spec} = supervisor:get_childspec(Pid, Child2),
+    {ok, Spec} = supervisor:get_childspec(Pid, Child3),
+    {error,not_found} = supervisor:get_childspec(Pid, self()),
+    terminate(Pid, shutdown).
+
 %%-------------------------------------------------------------------------
 %% Tests that the supervisor produces good error messages when start-
 %% and child specs are faulty.
-- 
cgit v1.2.3