Skip to content
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: Fix proc_lib:stop when exit reason is expected reason #8772

Merged

Conversation

garazdawi
Copy link
Contributor

When working with peer in #8537 I got a lot of errors where gen_server:stop would crash with reason normal as the peer process terminated with that reason just as gen_server:stop was called. I'm opening this PR to start a discussion to see if we want to make it so that if a process exits with the expected reason, though not was a result of the stop call, we should still consider it a successful shutdown?

@garazdawi garazdawi added team:VM Assigned to OTP team VM enhancement labels Sep 2, 2024
@garazdawi garazdawi self-assigned this Sep 2, 2024
Copy link
Contributor

github-actions bot commented Sep 2, 2024

CT Test Results

    2 files     95 suites   58m 7s ⏱️
2 156 tests 2 108 ✅ 48 💤 0 ❌
2 515 runs  2 465 ✅ 50 💤 0 ❌

Results for commit 3f2d85a.

♻️ 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

@IngelaAndin
Copy link
Contributor

I think it sounds reasonable, as the processes are parallel things that may stop execution by themselves or be shutdown by an other process and of course this can happen in the same time and there can be a race which one happens first, but they both end up with the wanted end result. I think it sound strange that we get a crash because something terminated normally!

@max-au
Copy link
Contributor

max-au commented Sep 2, 2024

Makes sense to me too (although changes existing behaviour).
If behaviour change isn't desirable, there could be a way to fix it specifically for peer.

@IngelaAndin
Copy link
Contributor

I think it comes down to if we consider the current behaviour to be a bug or not. If it is a bug I think it is ok to change it, we never promised to be bug-compatible.

@essen
Copy link
Contributor

essen commented Sep 3, 2024

This has bit me before so I 100% support the change. It's hard to imagine that this change will result in incompatibilities but I'm sure someone somewhere expects a normal crash.

@garazdawi
Copy link
Contributor Author

I'll merge it into 28 and mark it as a potential incompatability. I too have a hard time seeing someone relying on this.

@garazdawi garazdawi force-pushed the lukas/stdlib/proc_lib-stop-expected-reason branch from be42628 to 3f2d85a Compare September 11, 2024 10:53
@garazdawi garazdawi added the testing currently being tested, tag is used by OTP internal CI label Sep 11, 2024
@garazdawi garazdawi merged commit 41a43fe into erlang:master Sep 17, 2024
16 checks passed
@garazdawi garazdawi deleted the lukas/stdlib/proc_lib-stop-expected-reason branch September 17, 2024 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants