From b8d9edf97d8d647fe82d3fa9cbb564ad9174e42a Mon Sep 17 00:00:00 2001 From: Luke Imhoff Date: Thu, 27 Sep 2018 14:49:58 -0500 Subject: [PATCH] Indexer.Block.Catchup.BoundIntervalSupervisor is proper supervisor Fixes #592 Ensure that Indexer.Block.Catchup.BoundIntervalSupervisor acts as a proper Supervisor by ensure it supports all Supervisor and :supervisor functions. --- .../catchup/bound_interval_supervisor.ex | 121 ++++++++++++- .../bound_interval_supervisor_test.exs | 165 ++++++++++++++++++ 2 files changed, 282 insertions(+), 4 deletions(-) diff --git a/apps/indexer/lib/indexer/block/catchup/bound_interval_supervisor.ex b/apps/indexer/lib/indexer/block/catchup/bound_interval_supervisor.ex index ee356beca3..68f9ad7cd6 100644 --- a/apps/indexer/lib/indexer/block/catchup/bound_interval_supervisor.ex +++ b/apps/indexer/lib/indexer/block/catchup/bound_interval_supervisor.ex @@ -11,6 +11,8 @@ defmodule Indexer.Block.Catchup.BoundIntervalSupervisor do alias Indexer.{Block, BoundInterval} alias Indexer.Block.Catchup + @type named_arguments :: %{required(:block_fetcher) => Block.Fetcher.t(), optional(:block_interval) => pos_integer} + # milliseconds @block_interval 5_000 @@ -19,10 +21,17 @@ defmodule Indexer.Block.Catchup.BoundIntervalSupervisor do fetcher: %Catchup.Fetcher{}, task: nil - def child_spec(arg) do + @spec child_spec([named_arguments | GenServer.options(), ...]) :: Supervisor.child_spec() + def child_spec([named_arguments]) when is_map(named_arguments), do: child_spec([named_arguments, []]) + + def child_spec([named_arguments, gen_server_options] = start_link_arguments) + when is_map(named_arguments) and is_list(gen_server_options) do # The `child_spec` from `use Supervisor` because the one from `use GenServer` will set the `type` to `:worker` # instead of `:supervisor` and use the wrong shutdown timeout - Supervisor.child_spec(%{id: __MODULE__, start: {__MODULE__, :start_link, [arg]}, type: :supervisor}, []) + Supervisor.child_spec( + %{id: __MODULE__, start: {__MODULE__, :start_link, start_link_arguments}, type: :supervisor}, + [] + ) end @doc """ @@ -30,8 +39,10 @@ defmodule Indexer.Block.Catchup.BoundIntervalSupervisor do For `named_arguments` see `Indexer.BlockFetcher.new/1`. For `t:GenServer.options/0` see `GenServer.start_link/3`. """ - @spec start_link([named_arguments :: list() | GenServer.options()]) :: {:ok, pid} - def start_link([named_arguments, gen_server_options]) when is_map(named_arguments) and is_list(gen_server_options) do + @spec start_link(named_arguments :: map()) :: {:ok, pid} + @spec start_link(named_arguments :: %{}, GenServer.options()) :: {:ok, pid} + def start_link(named_arguments, gen_server_options \\ []) + when is_map(named_arguments) and is_list(gen_server_options) do GenServer.start_link(__MODULE__, named_arguments, gen_server_options) end @@ -57,6 +68,108 @@ defmodule Indexer.Block.Catchup.BoundIntervalSupervisor do } end + @impl GenServer + def handle_call(:count_children, _from, %__MODULE__{task: task} = state) do + active = + case task do + nil -> 0 + %Task{} -> 1 + end + + {:reply, [specs: 1, active: active, supervisors: 0, workers: 1], state} + end + + def handle_call({:delete_child, :task}, _from, %__MODULE__{task: task} = state) do + reason = + case task do + nil -> :restarting + %Task{} -> :running + end + + {:reply, {:error, reason}, state} + end + + def handle_call({:delete_child, _}, _from, %__MODULE__{} = state) do + {:reply, {:error, :not_found}, state} + end + + @task_shutdown_timeout 5_000 + + def handle_call({:get_childspec, :task}, _from, %__MODULE__{fetcher: %Catchup.Fetcher{} = catchup} = state) do + {:reply, + {:ok, + %{ + id: :task, + start: {Catchup.Fetcher, :task, [catchup]}, + restart: :transient, + shutdown: @task_shutdown_timeout, + type: :worker, + modules: [Catchup.Fetcher] + }}, state} + end + + def handle_call({:get_childspec, _}, _from, %__MODULE__{} = state) do + {:reply, {:error, :not_found}, state} + end + + def handle_call({:restart_child, :task}, _from, %__MODULE__{fetcher: %Catchup.Fetcher{} = catchup, task: nil} = state) do + %Task{pid: pid} = task = Task.Supervisor.async_nolink(Catchup.TaskSupervisor, Catchup.Fetcher, :task, [catchup]) + + {:reply, {:ok, pid}, %__MODULE__{state | task: task}} + end + + def handle_call({:restart_child, :task}, _from, %__MODULE__{task: %Task{}} = state) do + {:reply, {:error, :running}, state} + end + + def handle_call({:restart_child, _}, _from, %__MODULE__{} = state) do + {:reply, {:error, :not_found}, state} + end + + def handle_call({:start_child, %{id: :task}}, _from, %__MODULE__{task: nil} = state) do + {:reply, {:error, :already_present}, state} + end + + def handle_call({:start_child, %{id: :task}}, _from, %__MODULE__{task: %Task{pid: pid}} = state) do + {:reply, {:error, :already_present, pid}, state} + end + + def handle_call({:start_child, {:task, _, _, _, _, _}}, _from, %__MODULE__{task: nil} = state) do + {:reply, {:error, :already_present}, state} + end + + def handle_call({:start_child, {:task, _, _, _, _, _}}, _from, %__MODULE__{task: %Task{pid: pid}} = state) do + {:reply, {:error, :already_present, pid}, state} + end + + def handle_call({:start_child, _}, _from, %__MODULE__{} = state) do + {:reply, {:error, :not_supported}, state} + end + + def handle_call({:terminate_child, :task}, _from, %__MODULE__{task: nil} = state) do + {:reply, :ok, state} + end + + def handle_call({:terminate_child, :task}, _from, %__MODULE__{task: %Task{} = task} = state) do + Task.shutdown(task, @task_shutdown_timeout) + + {:reply, :ok, %__MODULE__{state | task: nil}} + end + + def handle_call({:terminate_child, _}, _from, %__MODULE__{} = state) do + {:reply, {:error, :not_found}, state} + end + + def handle_call(:which_children, _from, %__MODULE__{task: task} = state) do + child = + case task do + nil -> :restarting + %Task{pid: pid} -> pid + end + + {:reply, [{:task, child, :worker, [Catchup.Fetcher]}], state} + end + @impl GenServer def handle_info(:catchup_index, %__MODULE__{fetcher: %Catchup.Fetcher{} = catchup} = state) do {:noreply, diff --git a/apps/indexer/test/indexer/block/catchup/bound_interval_supervisor_test.exs b/apps/indexer/test/indexer/block/catchup/bound_interval_supervisor_test.exs index 02190c8138..0041ae51ae 100644 --- a/apps/indexer/test/indexer/block/catchup/bound_interval_supervisor_test.exs +++ b/apps/indexer/test/indexer/block/catchup/bound_interval_supervisor_test.exs @@ -228,6 +228,159 @@ defmodule Indexer.Block.Catchup.BoundIntervalSupervisorTest do end end + describe "Supervisor.count_children/1" do + setup :supervisor + + test "without task running returns 0 active", %{pid: pid} do + Supervisor.terminate_child(pid, :task) + + assert Supervisor.count_children(pid) == %{active: 0, specs: 1, supervisors: 0, workers: 1} + end + + test "with task running returns 1 active", %{pid: pid} do + assert Supervisor.count_children(pid) == %{active: 1, specs: 1, supervisors: 0, workers: 1} + end + end + + describe "Supervisor.delete_child/2" do + setup :supervisor + + test "with task running returns {:error, :running}", %{pid: pid} do + assert {:error, :running} = Supervisor.delete_child(pid, :task) + end + + test "without task running returns {:error, :restarting}", %{pid: pid} do + Supervisor.terminate_child(pid, :task) + + assert {:error, :restarting} = Supervisor.delete_child(pid, :task) + end + + test "with unknown child_id returns {:error, :not_found}", %{pid: pid} do + assert {:error, :not_found} = Supervisor.delete_child(pid, :other) + end + end + + describe ":supervisor.get_childspec/2" do + setup :supervisor + + test "with :task", %{pid: pid} do + assert {:ok, %{id: :task, modules: [Catchup.Fetcher], restart: _, shutdown: _, start: _, type: :worker}} = + :supervisor.get_childspec(pid, :task) + end + + test "with unknown child_id returns {:error, :not_found}", %{pid: pid} do + assert {:error, :not_found} = :supervisor.get_childspec(pid, :other) + end + end + + describe "Supervisor.restart_child/2" do + setup :supervisor + + test "without :task running restarts task", %{pid: pid} do + Supervisor.terminate_child(pid, :task) + + assert {:ok, child_pid} = Supervisor.restart_child(pid, :task) + + assert is_pid(child_pid) + end + + test "with :task running returns {:error, :running}", %{pid: pid} do + assert {:error, :running} = Supervisor.restart_child(pid, :task) + end + + test "with unknown child_id returns {:error, :not_found}", %{pid: pid} do + assert {:error, :not_found} = Supervisor.restart_child(pid, :other) + end + end + + describe "Supervisor.start_child/2" do + setup :supervisor + + test "with map with :task without running returns {:error, :already_present}", %{pid: pid} do + Supervisor.terminate_child(pid, :task) + + {:ok, child_spec} = :supervisor.get_childspec(pid, :task) + + assert is_map(child_spec) + + assert {:error, :already_present} = Supervisor.start_child(pid, child_spec) + end + + test "with map with :task with running returns {:error, :already_present, pid}", %{pid: pid} do + {:ok, child_spec} = :supervisor.get_childspec(pid, :task) + + assert is_map(child_spec) + + assert {:error, :already_present, child_pid} = Supervisor.start_child(pid, child_spec) + assert is_pid(child_pid) + end + + test "with tuple with :task without running returns {:error, :already_present}", %{pid: pid} do + Supervisor.terminate_child(pid, :task) + + {:ok, %{id: id, start: start, restart: restart, shutdown: shutdown, type: type, modules: modules}} = + :supervisor.get_childspec(pid, :task) + + assert {:error, :already_present} = Supervisor.start_child(pid, {id, start, restart, shutdown, type, modules}) + end + + test "with tuple with :task with running returns {:error, :already_present, pid}", %{pid: pid} do + {:ok, %{id: id, start: start, restart: restart, shutdown: shutdown, type: type, modules: modules}} = + :supervisor.get_childspec(pid, :task) + + assert {:error, :already_present, child_pid} = + Supervisor.start_child(pid, {id, start, restart, shutdown, type, modules}) + + assert is_pid(child_pid) + end + + test "with other child_spec returns {:error, :not_supported}", %{pid: pid} do + assert {:error, :not_supported} = Supervisor.start_child(pid, %{}) + end + end + + describe "Supervisor.terminate_child/2" do + setup :supervisor + + test "with :task without running returns :ok", %{pid: pid} do + Supervisor.terminate_child(pid, :task) + + assert :ok = Supervisor.terminate_child(pid, :task) + end + + test "with :task running returns :ok after shutting down child", %{pid: pid} do + [{:task, child_pid, _, _}] = Supervisor.which_children(pid) + + assert is_pid(child_pid) + + reference = Process.monitor(child_pid) + + assert :ok = Supervisor.terminate_child(pid, :task) + + assert_receive {:DOWN, ^reference, :process, ^child_pid, :shutdown} + end + + test "with other child_id returns {:error, :not_found}", %{pid: pid} do + assert {:error, :not_found} = Supervisor.terminate_child(pid, :other) + end + end + + describe "Supervisor.which_children/1" do + setup :supervisor + + test "without task running returns child as :restarting", %{pid: pid} do + Supervisor.terminate_child(pid, :task) + + assert [{:task, :restarting, :worker, [Catchup.Fetcher]}] = Supervisor.which_children(pid) + end + + test "with task running returns child as pid", %{pid: pid} do + assert [{:task, child_pid, :worker, [Catchup.Fetcher]}] = Supervisor.which_children(pid) + + assert is_pid(child_pid) + end + end + describe "handle_info(:catchup_index, state)" do setup context do # force to use `Mox`, so we can manipulate `lastest_block_number` @@ -376,4 +529,16 @@ defmodule Indexer.Block.Catchup.BoundIntervalSupervisorTest do %{state: state} end + + defp supervisor(%{json_rpc_named_arguments: json_rpc_named_arguments}) do + start_supervised!({Task.Supervisor, name: Indexer.Block.Catchup.TaskSupervisor}) + + pid = + start_supervised!( + {Catchup.BoundIntervalSupervisor, + [%{block_fetcher: %Indexer.Block.Fetcher{json_rpc_named_arguments: json_rpc_named_arguments}}]} + ) + + {:ok, %{pid: pid}} + end end