-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stdlib: supervisor unlink_flush improvement #8780
base: maint
Are you sure you want to change the base?
Conversation
u3s
commented
Sep 5, 2024
- handling race between orderly shutdown from supervisor termination + and worker termination
- make sure to report and correct error reason if possible
CT Test Results 2 files 95 suites 1h 0m 33s ⏱️ For more details on these failures, see this check. Results for commit abee2d9. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
2ceed3a
to
684f167
Compare
lib/stdlib/src/supervisor.erl
Outdated
after 0 -> | ||
NotExitReason | ||
end | ||
end; | ||
unlink_flush(Pid, DefaultReason) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to do something like this so as we want to change noproc = Reaosn to be a normal case as that is the race.
unlink_flush(Pid, Reaon) ->
%% We call unlink in order to guarantee that the 'EXIT' has arrived
%% from the dead process. See the unlink docs for details.
{links, Links} = process_info(self(), links),
case lists:member(Pid, Links) of
true ->
do_unlink_flush(Pid, Reason);
false -> %% Naughty child
flush(Pid, Reason)
end.
do_unlink_flush(Pid, noproc = Reason) ->
unlink(Pid),
receive
{'EXIT', Pid, Reason} ->
normal
end;
do_unlink_flush(Pid, Reason) ->
unlink(Pid),
receive
{'EXIT', Pid, Reason} ->
Reason
end.
flush(Pid, NoExit) ->
receive
{'EXIT', Pid, Reason} ->
Reason
after 0 ->
NoExit
end.
684f167
to
06738dd
Compare
@u3s @IngelaAndin since I was involved in bringing up the current |
@Maria-12648430 This is about a race condition when the application is being stopped, so supervisors are shutting down their children and children might be terminating on their own in the same time. Also the supervisor is trying to avoid hanging if a child process behaves bad and does unlink. So when a supervisor is terminating a child and gets |
We found a race condition taking place when supervision tree is terminated.
Above error comes from ssh test code which cleans up after execution - connection is closed by the client and server is stopped. we would like to remove the race condition in that particular case. |
Ok, understood, thanks for explaining 🤗 |
{links, Links} = process_info(self(), links), | ||
case lists:member(Pid, Links) of | ||
true -> | ||
do_unlink_flush(Pid, Reason); | ||
false -> %% Naughty child | ||
do_flush(Pid, Reason) | ||
end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question re this, though. Is it guaranteed that, if a process was linked to the supervisor and process_info(self(), links)
does not return its pid any more, the respective 'EXIT'
message is then in the mailbox of the supervisor? Because if not, there still remains an (admittedly very tiny) window for a race condition.
- handling race between orderly shutdown from supervisor termination + and worker termination - make sure to report and correct error reason if possible
06738dd
to
abee2d9
Compare
I've pushed some improvements but code is not working yet. |
I investigated it a bit and I think that the ide of actually making sure the EXIT has arrived is not feasible, it enforces a lot of receiving of messages that was before ignored. And in that case possible race that @Maria-12648430 though about does not matter either. So I am more thinking the solution might be something like this. But still this solution breaks one test case that
The thing that breaks is:
have to investigate further next week. |
Just a wild guess without having read anything else: you have |
Hm... I just tried, but can't reproduce the failure you mentioned. No matter if I run it with the current code, with your snippet as you posted it, or with my suggested correction on it: the test passes every time. |