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

Add field arbiter.stopping #2944

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vanschelven
Copy link
Contributor

Motivation: I'm running some code in which the server is stopped[1]. I would like to do this only when the server is not already in a "stopping" state. Because my code is tied to an event, it could be called "at any time", either because someone else already called server.stop, or in the example below even because our own call to server.stop caused the workers to quit. In any case, repeated calls to server.stop are undesirable because repeated execution of that method will lead to an exception. I'd like to instead check whether we're currently in the process of stopping.

def child_exit(server, worker):
    if not server.stopping:  # i.e. do this only once, not as a result of your own shutdown.
        server.log.info("worker with pid %s died, I am shutting down myself" % worker.pid)
        server.stop()

(my current solution works around the lack of self.stopping by monkey-patching def stop, but this is ugly)

[1] in this case the server is stopped whenever a child is stopped, but this is not material to the issue.

@@ -63,6 +63,7 @@ def __init__(self, app):
self.reexec_pid = 0
self.master_pid = 0
self.master_name = "Master"
self.stopping = False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure but may be is_stopping more better name for this, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably yes... but I'll wait for the more fundamental problems that @benoitc raised before dealing with the details

@benoitc
Copy link
Owner

benoitc commented Mar 9, 2023

I don't understand the use case. Can you provide an example? Why not simply add a hook executed when stopping?

@vanschelven
Copy link
Contributor Author

vanschelven commented Mar 10, 2023

I don't understand the use case. Can you provide an example?

I think I did? The code I have provided will not work pre-PR (it depends on if not server.stopping and such an attr does not exist) and if that if-statement is left-out, the server will crash in the process of shutting down.

Why not simply add a hook executed when stopping?

That depends on your definition of "simply"... in my world adding a flag is more simple than a hook which executes arbitrary code. Having said that such arbitrary code could always include the setting-of-the flag so it would indeed be a solution. And I can also see how it could be more simple in a world where all configuration is done with hooks (at least it would be more consistent).

@vanschelven
Copy link
Contributor Author

vanschelven commented Mar 10, 2023

because repeated execution of that method will lead to an exception

perhaps this was the bit you missed? I have to admit I did not write down at the time what the exception was and do not remember it exactly, but I can imagine that closing sockets more than once (which is what stop starts off doing) does not go down well

@benoitc
Copy link
Owner

benoitc commented May 7, 2023

related to #2884 I think we could have a generic way to allows the developer to define the behaviour when an instruction to stop Gunicorn is received. Let's keep the PR for now to get is as a basis to troduce an on_stopping hook function.

@benoitc benoitc added this to the 21.0 release milestone May 7, 2023
@tilgovi tilgovi modified the milestones: 21.0, 22.0 Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants