442037 (2) [Avatar] Offline
#1
The function ThySupervisor.restart_child/3 is supposed to take in the supervisor, the pid of the child, and the new child_spec for the child to be restarted with. However, once this call is translated to GenServer.call(supervisor, {:restart_child, pid, child_spec}) it calls handle_call({:restart_child, old_pid}, _from, state) without even including the new child_spec in the arguments. Where did the new child_spec go and why aren't we using it when calling handle_call({:restart_child, old_pid}, _from, state)? There isn't even an argument in the function signature to match it. Instead, handle_call/3 gets the child_spec from state using the old_pid of the process getting terminated as can be seen here:
def handle_call({:restart_child, old_pid}, _from, state) do
    case HashDict.fetch(state, old_pid) do
      {:ok, child_spec} ->
        case restart_child(old_pid, child_spec) do
          {:ok, {pid, child_spec}} ->
            new_state = state
                          |> HashDict.delete(old_pid)
                          |> HashDict.put(pid, child_spec)
          ...
    end
  end 

So, every time a child is restarted it is restarted with the exact same child_spec as it was started with even though we expressly pass a new one into it. Am I missing something here?

P.S. I belive that the header for this section which reads "RESTART_CHILD(PID, CHILD_SPEC)" is incorrect as the actual function signature is restart_child(supervisor, pid, child_spec) and clearly the arity is 3 not 2.

Edit: I have confirmed that the function signature at the very least is incorrect when calling GenServer.call(supervisor, {:restart_child, pid, child_spec}), in addition to the new child_spec never being utilized. Here's what I get in IEx when I attempt to restart a child process given the current source code on the GitHub repo:
iex(1)> {:ok, sup_pid} = ThySupervisor.start_link([])
{:ok, #PID<0.130.0>}
iex(2)> {:ok, child_pid} = ThySupervisor.start_child(sup_pid, {ThyWorker, :start_link, []})
{:ok, #PID<0.132.0>}
iex(3)> ThySupervisor.restart_child(sup_pid, child_pid, {ThyWorker, :start_link, []})
** (EXIT from #PID<0.127.0>) an exception was raised:
    ** (FunctionClauseError) no function clause matching in ThySupervisor.handle_call/3
        (thy_supervisor) lib/thy_supervisor.ex:45: ThySupervisor.handle_call({:restart_child, #PID<0.132.0>, {ThyWorker, :start_link, []}}, {#PID<0.127.0>, #Reference<0.0.3.750>}, %{#PID<0.132.0> => {ThyWorker, :start_link, []}})
        (stdlib) gen_server.erl:615: :gen_server.try_handle_call/4
        (stdlib) gen_server.erl:647: :gen_server.handle_msg/5
        (stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3


13:57:09.299 [error] GenServer #PID<0.130.0> terminating
** (FunctionClauseError) no function clause matching in ThySupervisor.handle_call/3
    (thy_supervisor) lib/thy_supervisor.ex:45: ThySupervisor.handle_call({:restart_child, #PID<0.132.0>, {ThyWorker, :start_link, []}}, {#PID<0.127.0>, #Reference<0.0.3.750>}, %{#PID<0.132.0> => {ThyWorker, :start_link, []}})
    (stdlib) gen_server.erl:615: :gen_server.try_handle_call/4
    (stdlib) gen_server.erl:647: :gen_server.handle_msg/5
    (stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3
Last message: {:restart_child, #PID<0.132.0>, {ThyWorker, :start_link, []}}
State: %{#PID<0.132.0> => {ThyWorker, :start_link, []}}
363169 (1) [Avatar] Offline
#2
Thank you for posting this about
:restart_child
, I just reached this part in the book and thought I was crazy. And if this works, I need someone to explain to me how - it seems like the function signature should never match, much less not allow you to pass in a new child_spec (looks like it will always use the old one)
421407 (2) [Avatar] Offline
#3
I just reached this point of the book too, and am quite confused. However, since the point of this section is to re-implement Supervisor from the stdlib I had a look at the official Elixir documentation for Supervisor and this is what I found:

https://hexdocs.pm/elixir/1.4.2/Supervisor.html#restart_child/2
restart_child(supervisor, child_id)


Therefore, I think the correct answer to our question is: the public restart_child function up in the # API section of the code is incorrect, and should be as follows:
#######
# API #
#######

def restart_child(supervisor, pid) when is_pid(pid) do
  GenServer.call(supervisor, {:restart_child, pid})
end


The rest of the code in the # Callback Functions and # Private Functions sections could remain the same, except for one little thing:—now we have a public and private restart_child/2 function. Hmmmmm. I decided the easiest thing to do here was to rename the private one to _restart_child/2, unfortunately this requires updating the handler callback code as well.
defp _restart_child(pid, child_spec) when is_pid(pid)


I think this solves the issue. That being said, I am reading the book along with the rest of us, so this is a case of the blind leading the blind smilie