From abce7ec25c23aded7e3bd8af5c56884bac05a608 Mon Sep 17 00:00:00 2001 From: Hannah Purcell Date: Wed, 27 Sep 2023 14:04:13 -0400 Subject: [PATCH 1/6] Explicitly expire child processes of a Task.Supervisor --- lib/screens/screens_by_alert/memcache.ex | 5 +++-- lib/screens/screens_by_alert/self_refresh_runner.ex | 5 +++-- lib/screens/util.ex | 12 ++++++++++++ 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/lib/screens/screens_by_alert/memcache.ex b/lib/screens/screens_by_alert/memcache.ex index 31f21ae14..b1d957638 100644 --- a/lib/screens/screens_by_alert/memcache.ex +++ b/lib/screens/screens_by_alert/memcache.ex @@ -27,6 +27,7 @@ defmodule Screens.ScreensByAlert.Memcache do ``` """ alias Screens.ScreensByAlert.Memcache.TaskSupervisor + alias Screens.Util require Logger @@ -56,10 +57,10 @@ defmodule Screens.ScreensByAlert.Memcache do # To avoid bottlenecks and unnecessarily blocking the caller, run in a separate task process _ = - Task.Supervisor.start_child(TaskSupervisor, fn -> + Task.Supervisor.start_child(TaskSupervisor, Util.fn_with_timeout(fn -> update_screens_last_updated_key(screen_id, now) Enum.each(alert_ids, &update_alert_key(&1, screen_id, now)) - end) + end, 10_000)) :ok end diff --git a/lib/screens/screens_by_alert/self_refresh_runner.ex b/lib/screens/screens_by_alert/self_refresh_runner.ex index 686b7dc47..adedb5d2b 100644 --- a/lib/screens/screens_by_alert/self_refresh_runner.ex +++ b/lib/screens/screens_by_alert/self_refresh_runner.ex @@ -6,6 +6,7 @@ defmodule Screens.ScreensByAlert.SelfRefreshRunner do alias Screens.Config.Screen alias Screens.ScreensByAlert + alias Screens.Util # (Not a real module--just a name assigned to the Task.Supervisor process that supervises each simulated data request run) alias Screens.ScreensByAlert.SelfRefreshRunner.TaskSupervisor @@ -70,9 +71,9 @@ defmodule Screens.ScreensByAlert.SelfRefreshRunner do # process from going down if an exception is raised while running # ScreenData.by_screen_id/1 for some screen. Enum.each(screen_ids_to_refresh, fn screen_id -> - Task.Supervisor.start_child(TaskSupervisor, fn -> + Task.Supervisor.start_child(TaskSupervisor, Util.fn_with_timeout(fn -> @screen_data_fn.(screen_id, skip_serialize: true) - end) + end, 10_000)) end) {:noreply, state} diff --git a/lib/screens/util.ex b/lib/screens/util.ex index a55771d18..374cad6d1 100644 --- a/lib/screens/util.ex +++ b/lib/screens/util.ex @@ -218,4 +218,16 @@ defmodule Screens.Util do def translate_carriage_occupancy_status(:full), do: :crowded def translate_carriage_occupancy_status(:not_accepting_passengers), do: :closed def translate_carriage_occupancy_status(_), do: nil + + @doc """ + Adds a timeout to a function. Mainly used for child processes of a Task.Supervisor + which don't come with a timeout by default. + """ + @spec fn_with_timeout((-> val), non_neg_integer()) :: (-> val) when val: any() + def fn_with_timeout(fun, timeout) do + fn -> + :timer.exit_after(timeout, :normal) + fun.() + end + end end From d61eb2ffaea7633df13af2dac8717cfaa54430fb Mon Sep 17 00:00:00 2001 From: Hannah Purcell Date: Wed, 27 Sep 2023 14:20:00 -0400 Subject: [PATCH 2/6] formatting --- lib/screens/screens_by_alert/memcache.ex | 14 ++++++++++---- .../screens_by_alert/self_refresh_runner.ex | 12 +++++++++--- lib/screens/util.ex | 2 +- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/lib/screens/screens_by_alert/memcache.ex b/lib/screens/screens_by_alert/memcache.ex index b1d957638..4dbbd4a5d 100644 --- a/lib/screens/screens_by_alert/memcache.ex +++ b/lib/screens/screens_by_alert/memcache.ex @@ -57,10 +57,16 @@ defmodule Screens.ScreensByAlert.Memcache do # To avoid bottlenecks and unnecessarily blocking the caller, run in a separate task process _ = - Task.Supervisor.start_child(TaskSupervisor, Util.fn_with_timeout(fn -> - update_screens_last_updated_key(screen_id, now) - Enum.each(alert_ids, &update_alert_key(&1, screen_id, now)) - end, 10_000)) + Task.Supervisor.start_child( + TaskSupervisor, + Util.fn_with_timeout( + fn -> + update_screens_last_updated_key(screen_id, now) + Enum.each(alert_ids, &update_alert_key(&1, screen_id, now)) + end, + 10_000 + ) + ) :ok end diff --git a/lib/screens/screens_by_alert/self_refresh_runner.ex b/lib/screens/screens_by_alert/self_refresh_runner.ex index adedb5d2b..1b1f63e89 100644 --- a/lib/screens/screens_by_alert/self_refresh_runner.ex +++ b/lib/screens/screens_by_alert/self_refresh_runner.ex @@ -71,9 +71,15 @@ defmodule Screens.ScreensByAlert.SelfRefreshRunner do # process from going down if an exception is raised while running # ScreenData.by_screen_id/1 for some screen. Enum.each(screen_ids_to_refresh, fn screen_id -> - Task.Supervisor.start_child(TaskSupervisor, Util.fn_with_timeout(fn -> - @screen_data_fn.(screen_id, skip_serialize: true) - end, 10_000)) + Task.Supervisor.start_child( + TaskSupervisor, + Util.fn_with_timeout( + fn -> + @screen_data_fn.(screen_id, skip_serialize: true) + end, + 10_000 + ) + ) end) {:noreply, state} diff --git a/lib/screens/util.ex b/lib/screens/util.ex index 374cad6d1..4c5ebbab4 100644 --- a/lib/screens/util.ex +++ b/lib/screens/util.ex @@ -223,7 +223,7 @@ defmodule Screens.Util do Adds a timeout to a function. Mainly used for child processes of a Task.Supervisor which don't come with a timeout by default. """ - @spec fn_with_timeout((-> val), non_neg_integer()) :: (-> val) when val: any() + @spec fn_with_timeout((() -> val), non_neg_integer()) :: (() -> val) when val: any() def fn_with_timeout(fun, timeout) do fn -> :timer.exit_after(timeout, :normal) From 07ddc0fad8b199587643123293b0d88daed6f034 Mon Sep 17 00:00:00 2001 From: Hannah Purcell Date: Wed, 27 Sep 2023 14:33:31 -0400 Subject: [PATCH 3/6] dialyzer fix --- lib/screens/util.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/screens/util.ex b/lib/screens/util.ex index 4c5ebbab4..6d7f4d210 100644 --- a/lib/screens/util.ex +++ b/lib/screens/util.ex @@ -226,7 +226,7 @@ defmodule Screens.Util do @spec fn_with_timeout((() -> val), non_neg_integer()) :: (() -> val) when val: any() def fn_with_timeout(fun, timeout) do fn -> - :timer.exit_after(timeout, :normal) + _ = :timer.exit_after(timeout, :normal) fun.() end end From 79ccad3349b869fd7419012ea58517f3b138ce96 Mon Sep 17 00:00:00 2001 From: Hannah Purcell Date: Wed, 27 Sep 2023 16:20:02 -0400 Subject: [PATCH 4/6] Also make a fallback exit for dynamicsupervisor --- lib/screens/ol_crowding/dynamic_supervisor.ex | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/screens/ol_crowding/dynamic_supervisor.ex b/lib/screens/ol_crowding/dynamic_supervisor.ex index e8bf4edd9..352c99af3 100644 --- a/lib/screens/ol_crowding/dynamic_supervisor.ex +++ b/lib/screens/ol_crowding/dynamic_supervisor.ex @@ -48,6 +48,7 @@ defmodule Screens.OlCrowding.DynamicSupervisor do restart: :transient } - DynamicSupervisor.start_child(__MODULE__, spec) + {:ok, child_pid} = DynamicSupervisor.start_child(__MODULE__, spec) + _ = :timer.exit_after(10_000, child_pid, :kill) end end From 2574cee59c93abcf33d8f6f5ab5eaa35b989e7a3 Mon Sep 17 00:00:00 2001 From: Hannah Purcell <69368883+hannahpurcell@users.noreply.github.com> Date: Wed, 27 Sep 2023 16:21:05 -0400 Subject: [PATCH 5/6] To truly exit process, must kill with fire Co-authored-by: Jon Zimbel <63608771+jzimbel-mbta@users.noreply.github.com> --- lib/screens/util.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/screens/util.ex b/lib/screens/util.ex index 6d7f4d210..4b6111e88 100644 --- a/lib/screens/util.ex +++ b/lib/screens/util.ex @@ -226,7 +226,7 @@ defmodule Screens.Util do @spec fn_with_timeout((() -> val), non_neg_integer()) :: (() -> val) when val: any() def fn_with_timeout(fun, timeout) do fn -> - _ = :timer.exit_after(timeout, :normal) + _ = :timer.exit_after(timeout, :kill) fun.() end end From 46030728c656f187c72c44eee469a72ed6abbd74 Mon Sep 17 00:00:00 2001 From: Hannah Purcell Date: Wed, 27 Sep 2023 16:41:19 -0400 Subject: [PATCH 6/6] If process fails to start, handle it more gracefully --- lib/screens/ol_crowding/dynamic_supervisor.ex | 23 +++++++++++++++---- lib/screens/ol_crowding/logger.ex | 2 +- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/screens/ol_crowding/dynamic_supervisor.ex b/lib/screens/ol_crowding/dynamic_supervisor.ex index 352c99af3..c72c47d6a 100644 --- a/lib/screens/ol_crowding/dynamic_supervisor.ex +++ b/lib/screens/ol_crowding/dynamic_supervisor.ex @@ -1,8 +1,10 @@ defmodule Screens.OlCrowding.DynamicSupervisor do @moduledoc false + require Logger + use DynamicSupervisor - alias Screens.OlCrowding.Logger + alias Screens.OlCrowding.LogCrowdingInfo def start_link(init_arg) do DynamicSupervisor.start_link(__MODULE__, init_arg, name: __MODULE__) @@ -27,9 +29,9 @@ defmodule Screens.OlCrowding.DynamicSupervisor do fetch_params ) do spec = %{ - id: Logger, + id: LogCrowdingInfo, start: - {Logger, :start_link, + {LogCrowdingInfo, :start_link, [ %{ original_crowding_levels: original_crowding_levels, @@ -48,7 +50,18 @@ defmodule Screens.OlCrowding.DynamicSupervisor do restart: :transient } - {:ok, child_pid} = DynamicSupervisor.start_child(__MODULE__, spec) - _ = :timer.exit_after(10_000, child_pid, :kill) + case DynamicSupervisor.start_child(__MODULE__, spec) do + {:ok, child_pid} -> + _ = :timer.exit_after(10_000, child_pid, :kill) + + {:ok, child_pid, _} -> + _ = :timer.exit_after(10_000, child_pid, :kill) + + {:error, error} -> + Logger.error("crowding_dyn_supervisor_process_error #{inspect(error)}") + + _ -> + Logger.warn("Something went wrong with starting the crowding dynamic supervisor process") + end end end diff --git a/lib/screens/ol_crowding/logger.ex b/lib/screens/ol_crowding/logger.ex index 287e4b204..d2c41ae28 100644 --- a/lib/screens/ol_crowding/logger.ex +++ b/lib/screens/ol_crowding/logger.ex @@ -1,4 +1,4 @@ -defmodule Screens.OlCrowding.Logger do +defmodule Screens.OlCrowding.LogCrowdingInfo do @moduledoc false require Logger