-
-
Notifications
You must be signed in to change notification settings - Fork 335
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
Fix halfway-stopping of listeners #348
base: master
Are you sure you want to change the base?
Conversation
* if the process calling ranch:stop_listener crashes before finishing, the stopping is still exetuted completely * if a listener is terminated but not deleted, calling ranch:stop_listener removes the remnant
Hi Viktor, long time no see 😅
Yes, that would be a blueprint for a test case for my second point, when a listener already is in the halfway-stopped state. The hard-to-test part revolves around my first point, which should make it even impossible to put a listener in that state by "normal means" and crash at unfortunate times =^^= |
@essen? WDYT of the approach I used and the PR in general? |
Why isn't this done in |
Sure, could be done, not a bad idea :) I'll make a draft to show to you, maybe tomorrow. |
... or maybe today =^^= WDYT? |
I would not move the stop logic to Something like |
Ah, ok, so you mean leave the stopping code in the The way you propose would enable anyone to run any code in the
|
Yes.
We can restrict the M and F to ranch and do_stop_listener or whatever the name is, for now. |
@essen What's the point of this extra abstraction? To me it looks like over-engineering. Why not just do the simple protection (or workaround if consider it an OTP bug) for the fact that |
We need to do it in a process that won't be the caller, it can either be a random process that we spawn for this specifically, or Not sure which part is over engineering. |
If |
Thanks, makes sense. |
1c3b4f7
to
de6d4b6
Compare
de6d4b6
to
a2c959e
Compare
Looks good, thanks! I'm off for the next two weeks but I'll look into merging and releasing a new version when I get back. |
Have a nice vacation then 👋 |
@essen what about this PR? 3+ months have passed 😅 |
Fixes #347.
The changes in this PR actually do two things:
ranch:stop_listener
at an unfortunate time (namely, between thesupervisor:terminate_child
andsupervisor:delete_child
calls involved) should no longer prevent the stopping procedure from going to completionThere are no tests yet (and for the former case, I don't see how we could reliably create the scenario of the calling process crashing at just the right time). I want to hear what you think about this approach first before investing more time.