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

[Question] - Worker signal send behavior #2604

Closed
Kludex opened this issue Jul 6, 2021 · 13 comments
Closed

[Question] - Worker signal send behavior #2604

Kludex opened this issue Jul 6, 2021 · 13 comments

Comments

@Kludex
Copy link

Kludex commented Jul 6, 2021

Hi there! 👋

When gunicorn receives a SIGINT and SIGQUIT, it sends a SIGQUIT to the worker, instead of sending the same signal that it received. Is that on purpose?

Related snippets:

def handle_int(self):
"SIGINT handling"
self.stop(False)
raise StopIteration
def handle_quit(self):
"SIGQUIT handling"
self.stop(False)
raise StopIteration

if not graceful:
sig = signal.SIGQUIT

@benoitc
Copy link
Owner

benoitc commented Jul 6, 2021

yes. on SIGINT we really want to terminate the process and clear the resources . SIGTERM is used for graceful shutdown. Also it's more simple that way.

https://programmergamer.blogspot.com/2013/05/clarification-on-sigint-sigterm-sigkill.html

@Kludex
Copy link
Author

Kludex commented Jul 6, 2021

Hey @benoitc , thanks for the reply!

I guess the question I meant is more: why not forwarding the SIGINT gunicorn receives to the worker?

My issue is that we don't handle SIGQUIT on uvicorn, which we could, but I don't understand why the forwarding doesn't happen. Would you mind enlighten me?

@benoitc
Copy link
Owner

benoitc commented Jul 6, 2021 via email

@Kludex
Copy link
Author

Kludex commented Jul 6, 2021

There is some history behind. I will look at my notes and come back to you
later this evening.

Thanks! It's really appreciated. 👍

Maybe you could handle both cases? In any case, I think it should be
expected that SIGINT behaves like SIGQUIT and terminate immediately the
workers .

I guess. What we do is: SIGTERM and SIGINT give the time to the worker to shutdown gracefully, and if those signals are send again, it terminates immediately. I understand that they should not behave the same way, and SIGINT should terminate immediately. I'll discuss to other maintainers and look to why that decision was made.

Anyway, I'll be looking forward to what you can find about the different forwarding signal. Thanks for the efforts! 😄

@sandys
Copy link

sandys commented Jul 10, 2021

I guess. What we do is: SIGTERM and SIGINT give the time to the worker to shutdown gracefully, and if those signals are send again, it terminates immediately. I understand that they should not behave the same way, and SIGINT should terminate immediately. I'll discuss to other maintainers and look to why that decision was made.

Anyway, I'll be looking forward to what you can find about the different forwarding signal. Thanks for the efforts! 😄

@Kludex thanks for bringing up this issue. there is a long awaited bug #2339 which manifests in uvicorn worker. If you're filing a bug on uvicorn about this, could you please link here ?

@Kludex
Copy link
Author

Kludex commented Jul 10, 2021

@sandys I've created an issue on uvicorn, but it's not the issue presented here. It's only related.
I still want to understand the signal forwarding happening here. 😢

@HansBrende
Copy link

HansBrende commented Oct 27, 2021

I guess. What we do is: SIGTERM and SIGINT give the time to the worker to shutdown gracefully, and if those signals are send again, it terminates immediately. I understand that they should not behave the same way, and SIGINT should terminate immediately. I'll discuss to other maintainers and look to why that decision was made.

@Kludex did you? I am curious why that decision was made...
For some reason whenever I click "stop" in IntelliJ it sends a SIGINT, and then a SIGTERM immediately afterwards (read: < 0.1 ms afterwards)... causing a very undignified uvicorn exit. (It will later send a SIGKILL if you click "stop" a second time). It would be nice if I didn't get a flood of asyncio.CancelledError and "Task was destroyed but it is pending!" every time I exit. (No clue why the SIGINT happens before the SIGTERM, either... seems counterintuitive). I have created my own monkey patch to fix this annoyance which simply resets force_exit to false on every handle_exit call.

@Kludex
Copy link
Author

Kludex commented Oct 27, 2021

I'm not sure you're talking about the same issue. The problem here is forwarding SIGQUIT.

Answering your question, I've studied more about the issue by now, and I think the decision about handling SIGINT and SIGTERM in unicorn the same way is fine.

@HansBrende
Copy link

@Kludex the decision is not great for IDEs such as intellij due to the aforementioned problems. I do like the behavior of not force terminating on the first signal, but wouldn't it be better to only force terminate when the 2nd signal that comes thru is a SIGINT, not a sigterm? That would fix the IDE problem. I will create a new uvicorn issue for this if you think it is a good suggestion.

@Kludex
Copy link
Author

Kludex commented Oct 27, 2021

You're deviating the topic of this issue 😗

Feel free to create an issue on uvicorn or talk with the members on our gitter chat.

@HansBrende
Copy link

I will create a new uvicorn issue for this if you think it is a good suggestion.

@benoitc
Copy link
Owner

benoitc commented Oct 14, 2022

i am not sure what forwarding SIGINT means, but the reason we send a separate signal is that the arbiter acts as a kin dof supervisor. I am closing it as it' an aold question. Feel free to join me directly if you want to discuss it.

@benoitc benoitc closed this as not planned Won't fix, can't repro, duplicate, stale Oct 14, 2022
@Kludex
Copy link
Author

Kludex commented Oct 31, 2022

My goal here was to understand gunicorn's logic there, so I can act on uvicorn accordingly.

[...] the reason we send a separate signal is that the arbiter acts as a kin dof supervisor.

I see this more as an excuse to way this is not something gunicorn should be worried (which is fine), than a reasoning for the decision that was taken there.

In any case, I'll modify the UvicornWorker worker, and add a SIGQUIT handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants