benoitc / gunicorn

gunicorn 'Green Unicorn' is a WSGI HTTP Server for UNIX, fast clients and sleepy applications.
http://www.gunicorn.org
Other
9.87k stars 1.75k forks source link

arbiter: Wait for all old workers to terminate on SIGHUP #3312

Open sylt opened 1 month ago

sylt commented 1 month ago

This is to prevent them getting double SIGTERM signals sent to them (once the main loop runs self.manage_workers again and notices that there are too many workers spawned). It seems as if they exit due to signal termination in case they are sent SIGTERM, but are not yet reaped.

Hopefully solves #3050.

benoitc commented 1 month ago

thanks for the pr. I will check it.

however the need for this looks odd for me, this should have been handled bh the engine already. I will review recent changes to check.

sylt commented 1 month ago

Yeah, maybe it is odd. Perhaps there are better ways to fix it? I can agree that this is a bit of a band-aid.

Just for the record, in order for you to make an independent analysis of the problem, this is how I've reproduced the ERROR logs:

# In one console, start the example standalone app:
python examples/standalone_app.py

# In another console, issue a HUP to the main gunicorn process:
pkill -f --oldest --signal HUP '.*python.*examples/standalone_app.py'

I think this "error" has existed for ages, but it has never been caught due to that it wasn't logged as an error before. E.g., it couldn't be reproduced 20.0.0, but if you add the commit (28df9926d) which just adds the logging of the error, you'll see it.

There are other, more obscure, ways to achieve this error as well. Here is an example using the standalone app again:

# Rapidly, make one worker exit, and one worker start:
for sig in TTOU TTIN; do 
  pkill -f --oldest --signal $sig '.*python.*examples/standalone_app.py'; 
done

And to be clear, this PR will not fix this case above. I have tried to fix the more general problem in https://github.com/sylt/gunicorn/pull/1, but that unfortunately relies on work done in #3148, even thought I guess it would be portable.