[fix] Explicitly expire child processes of a Task.Supervisor #1873

hannahpurcell · 2023-09-27T18:08:53Z

Child processes of Task.Supervisors do not expire, even if they are meant to be temporary. We need to explicitly end the process if it's running too long. At the moment, the timeout is set for 10s.

github-actions · 2023-09-27T18:41:44Z

Coverage of commit `07ddc0f`

Summary coverage rate:
  lines......: 40.5% (2296 of 5674 lines)
  functions..: 42.1% (1052 of 2501 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/screens_by_alert/memcache.ex                                 | 0.0%     45| 0.0%    12|    -      0
  lib/screens/screens_by_alert/self_refresh_runner.ex                      |15.0%     20|50.0%     6|    -      0
  lib/screens/util.ex                                                      |64.8%     71|72.2%    18|    -      0

Download coverage report

cmaddox5 · 2023-09-27T18:49:04Z

Should the fallback for the DynamicSupervisor we discussed in Slack also be a part of this change? I don't think that fallback can be applied the same way as these, but will be a similar enough change for it to be in scope for this PR.

jzimbel-mbta

Good to go, just need that one change to the exit reason in order to force the process to stop.

lib/screens/util.ex

hannahpurcell · 2023-09-27T20:10:25Z

Should the fallback for the DynamicSupervisor we discussed in Slack also be a part of this change?

@cmaddox5 Sure! I lost sight of that once we found the more realistic bug. I'm having trouble thinking through the erlang process driven logic. Would this just be adding a direct call to :timer.kill_after() right after the start_child line?

cmaddox5 · 2023-09-27T20:14:05Z

Should the fallback for the DynamicSupervisor we discussed in Slack also be a part of this change?

@cmaddox5 Sure! I lost sight of that once we found the more realistic bug. I'm having trouble thinking through the erlang process driven logic. Would this just be adding a direct call to :timer.kill_after() right after the start_child line?

I think it might be safer to get the pid from the return of start_child and call kill_after on that pid. So instead using the timer function before, it would be after in this particular case.

hannahpurcell · 2023-09-27T20:15:49Z

I think it might be safer to get the pid from the return of start_child

Ooohhh of course it returns a pid. Ok ✍️

Co-authored-by: Jon Zimbel <[email protected]>

github-actions · 2023-09-27T20:25:36Z

Coverage of commit `2574cee`

Summary coverage rate:
  lines......: 40.5% (2296 of 5675 lines)
  functions..: 42.1% (1052 of 2501 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/ol_crowding/dynamic_supervisor.ex                            |40.0%      5|66.7%     3|    -      0
  lib/screens/screens_by_alert/memcache.ex                                 | 0.0%     45| 0.0%    12|    -      0
  lib/screens/screens_by_alert/self_refresh_runner.ex                      |15.0%     20|50.0%     6|    -      0
  lib/screens/util.ex                                                      |64.8%     71|72.2%    18|    -      0

Download coverage report

cmaddox5 · 2023-09-27T20:26:26Z

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)


In order to avoid crashing the app when one of these fails to start, can this be handled in a case instead and if the result is anything but {:ok, child_pid} we can just fail silently (or with a log, up to you on that)?

mmmm mmhmm 👍 👍

github-actions · 2023-09-27T20:27:10Z

Coverage of commit `2574cee`

Summary coverage rate:
  lines......: 40.5% (2296 of 5675 lines)
  functions..: 42.1% (1052 of 2501 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/ol_crowding/dynamic_supervisor.ex                            |40.0%      5|66.7%     3|    -      0
  lib/screens/screens_by_alert/memcache.ex                                 | 0.0%     45| 0.0%    12|    -      0
  lib/screens/screens_by_alert/self_refresh_runner.ex                      |15.0%     20|50.0%     6|    -      0
  lib/screens/util.ex                                                      |64.8%     71|72.2%    18|    -      0

Download coverage report

…eens into hp/expire-old-processes

github-actions · 2023-09-27T20:50:06Z

Coverage of commit `db2eac7`

Summary coverage rate:
  lines......: 40.4% (2296 of 5678 lines)
  functions..: 42.1% (1052 of 2501 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/ol_crowding/dynamic_supervisor.ex                            |25.0%      8|66.7%     3|    -      0
  lib/screens/ol_crowding/logger.ex                                        | 0.0%     18| 0.0%     4|    -      0
  lib/screens/screens_by_alert/memcache.ex                                 | 0.0%     45| 0.0%    12|    -      0
  lib/screens/screens_by_alert/self_refresh_runner.ex                      |15.0%     20|50.0%     6|    -      0
  lib/screens/util.ex                                                      |64.8%     71|72.2%    18|    -      0

Download coverage report

Explicitly expire child processes of a Task.Supervisor

abce7ec

hannahpurcell requested review from a team, PaulJKim and cmaddox5 and removed request for a team and PaulJKim September 27, 2023 18:08

hannahpurcell added 2 commits September 27, 2023 14:20

formatting

d61eb2f

dialyzer fix

07ddc0f

jzimbel-mbta approved these changes Sep 27, 2023

View reviewed changes

lib/screens/util.ex Outdated Show resolved Hide resolved

hannahpurcell and others added 2 commits September 27, 2023 16:20

Also make a fallback exit for dynamicsupervisor

79ccad3

To truly exit process, must kill with fire

2574cee

Co-authored-by: Jon Zimbel <[email protected]>

cmaddox5 reviewed Sep 27, 2023

View reviewed changes

hannahpurcell added 2 commits September 27, 2023 16:41

If process fails to start, handle it more gracefully

4603072

Merge branch 'hp/expire-old-processes' of https://github.com/mbta/scr…

db2eac7

…eens into hp/expire-old-processes

cmaddox5 approved these changes Sep 27, 2023

View reviewed changes

cmaddox5 assigned hannahpurcell Sep 27, 2023

hannahpurcell merged commit e9e4e67 into master Sep 27, 2023
2 checks passed

hannahpurcell deleted the hp/expire-old-processes branch September 27, 2023 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Explicitly expire child processes of a Task.Supervisor #1873

[fix] Explicitly expire child processes of a Task.Supervisor #1873

hannahpurcell commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

cmaddox5 commented Sep 27, 2023

jzimbel-mbta left a comment

hannahpurcell commented Sep 27, 2023 •

edited

Loading

cmaddox5 commented Sep 27, 2023

hannahpurcell commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

cmaddox5 Sep 27, 2023

hannahpurcell Sep 27, 2023

github-actions bot commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

[fix] Explicitly expire child processes of a Task.Supervisor #1873

[fix] Explicitly expire child processes of a Task.Supervisor #1873

Conversation

hannahpurcell commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

Coverage of commit 07ddc0f

cmaddox5 commented Sep 27, 2023

jzimbel-mbta left a comment

Choose a reason for hiding this comment

hannahpurcell commented Sep 27, 2023 • edited Loading

cmaddox5 commented Sep 27, 2023

hannahpurcell commented Sep 27, 2023

github-actions bot commented Sep 27, 2023

Coverage of commit 2574cee

cmaddox5 Sep 27, 2023

Choose a reason for hiding this comment

hannahpurcell Sep 27, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 27, 2023

Coverage of commit 2574cee

github-actions bot commented Sep 27, 2023

Coverage of commit db2eac7

Coverage of commit `07ddc0f`

hannahpurcell commented Sep 27, 2023 •

edited

Loading

Coverage of commit `2574cee`

Coverage of commit `2574cee`

Coverage of commit `db2eac7`