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

Make the gevent workers handle the quit signal by deferring to a new greenlet #1128

Merged
merged 1 commit into from Oct 26, 2015

Conversation

Projects
None yet
5 participants
@jamadden
Contributor

jamadden commented Oct 15, 2015

As discussed in #1126. This fixes the problem interactively. However, I have no idea how to add a unit test; there doesn't seem to be anything similar in place already. Suggestions welcome!

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 15, 2015

Collaborator

Hmmm. I'm actually wondering if we shouldn't just remove the time.sleep(). I know I tried to explain why I thought we had it, but I may not actually agree with it :). What do you think, @jamadden, or anyone else?

Collaborator

tilgovi commented Oct 15, 2015

Hmmm. I'm actually wondering if we shouldn't just remove the time.sleep(). I know I tried to explain why I thought we had it, but I may not actually agree with it :). What do you think, @jamadden, or anyone else?

@jamadden

This comment has been minimized.

Show comment
Hide comment
@jamadden

jamadden Oct 16, 2015

Contributor

I'm +1 for simply removing time.sleep. It's not like it's been working, and we haven't noticed any particular issues simply because the worker exits without sleeping.

Contributor

jamadden commented Oct 16, 2015

I'm +1 for simply removing time.sleep. It's not like it's been working, and we haven't noticed any particular issues simply because the worker exits without sleeping.

@xtao

This comment has been minimized.

Show comment
Hide comment
@xtao

xtao Oct 16, 2015

It works, thanks.

xtao commented Oct 16, 2015

It works, thanks.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Oct 16, 2015

Owner

@tilgovi it's here to let some times to the VM to release locks.

Patch looks good. However I am wondering if there isn't a way to patch the signal handler when launching the worker. Indeed gevent.spawn is quite the same as a gevent.sleep or time.sleep when patched. I think it would worth to investigate this possibility imo.

Owner

benoitc commented Oct 16, 2015

@tilgovi it's here to let some times to the VM to release locks.

Patch looks good. However I am wondering if there isn't a way to patch the signal handler when launching the worker. Indeed gevent.spawn is quite the same as a gevent.sleep or time.sleep when patched. I think it would worth to investigate this possibility imo.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 16, 2015

Collaborator

Gevent < 1.1 does not monkey patch signal, but even with 1.1 that patches signal.signal and time.sleep the same error occurs.

Collaborator

tilgovi commented Oct 16, 2015

Gevent < 1.1 does not monkey patch signal, but even with 1.1 that patches signal.signal and time.sleep the same error occurs.

@jamadden

This comment has been minimized.

Show comment
Hide comment
@jamadden

jamadden Oct 16, 2015

Contributor

@tilgovi Signals are delivered to the main greenlet, which happens to be the one running the hub---so the net result is the same on 1.0. So a complete stack trace captured there actually looks like this (captured with pdb):

[2015-10-16 07:34:43 -0500] [20467] [INFO] Booting worker with pid: 20467
^C[2015-10-16 07:34:45 -0500] [20464] [INFO] Handling signal: int
> //gunicorn/gunicorn/workers/ggevent.py(175)handle_quit()
-> gevent.spawn(super(GeventWorker, self).handle_quit, sig, frame)
(Pdb) bt
  //lib/python2.7/site-packages/gevent/hub.py(371)run()
-> loop.run()
  //gunicorn/gunicorn/workers/ggevent.py(174)handle_quit()
-> pdb.set_trace()

Under 1.1, monkey patching signal only has any effect for SIGCHLD; everything else is left untouched and behaves just as it did in 1.0.

@benoitc gevent.spawn is not the same thing as gevent.sleep or time.sleep. Both of the latter are blocking functions, while spawn is not. spawn is explicitly documented as safe to use in situations like this:

Note that the callbacks supplied to the libev API are run in the gevent.hub.Hub greenlet and thus cannot use the synchronous gevent API. It is possible to use the asynchronous API there, like gevent.spawn() and gevent.event.Event.set().

gevent does have a function gevent.signal that has the same effect as this patch (runs the handler in a new greenlet), but that API must be invoked directly---it is not monkey-patched in for compatibility.

Contributor

jamadden commented Oct 16, 2015

@tilgovi Signals are delivered to the main greenlet, which happens to be the one running the hub---so the net result is the same on 1.0. So a complete stack trace captured there actually looks like this (captured with pdb):

[2015-10-16 07:34:43 -0500] [20467] [INFO] Booting worker with pid: 20467
^C[2015-10-16 07:34:45 -0500] [20464] [INFO] Handling signal: int
> //gunicorn/gunicorn/workers/ggevent.py(175)handle_quit()
-> gevent.spawn(super(GeventWorker, self).handle_quit, sig, frame)
(Pdb) bt
  //lib/python2.7/site-packages/gevent/hub.py(371)run()
-> loop.run()
  //gunicorn/gunicorn/workers/ggevent.py(174)handle_quit()
-> pdb.set_trace()

Under 1.1, monkey patching signal only has any effect for SIGCHLD; everything else is left untouched and behaves just as it did in 1.0.

@benoitc gevent.spawn is not the same thing as gevent.sleep or time.sleep. Both of the latter are blocking functions, while spawn is not. spawn is explicitly documented as safe to use in situations like this:

Note that the callbacks supplied to the libev API are run in the gevent.hub.Hub greenlet and thus cannot use the synchronous gevent API. It is possible to use the asynchronous API there, like gevent.spawn() and gevent.event.Event.set().

gevent does have a function gevent.signal that has the same effect as this patch (runs the handler in a new greenlet), but that API must be invoked directly---it is not monkey-patched in for compatibility.

@jamadden

This comment has been minimized.

Show comment
Hide comment
@jamadden

jamadden Oct 16, 2015

Contributor

@benoitc I'm curious, can you share some more details about what locks you're talking about? And why they wouldn't be automatically cleaned up by interpreter shut-down? Is this something gunicorn specific?

Contributor

jamadden commented Oct 16, 2015

@benoitc I'm curious, can you share some more details about what locks you're talking about? And why they wouldn't be automatically cleaned up by interpreter shut-down? Is this something gunicorn specific?

@jamadden

This comment has been minimized.

Show comment
Hide comment
@jamadden

jamadden Oct 19, 2015

Contributor

Any more thoughts on this?

Contributor

jamadden commented Oct 19, 2015

Any more thoughts on this?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 19, 2015

Collaborator

From what I can tell, gevent 1.1 does monkey patch signal.signal so I'm a bit confused why this problem still exists there.

Collaborator

tilgovi commented Oct 19, 2015

From what I can tell, gevent 1.1 does monkey patch signal.signal so I'm a bit confused why this problem still exists there.

@jamadden

This comment has been minimized.

Show comment
Hide comment
@jamadden

jamadden Oct 19, 2015

Contributor

Yes, there is a monkey patch on signal.signal in 1.1, but it only handles the case where the first argument is SIGCHLD. Everything else is passed through to the standard library, including SIGQUIT and SIGINT. So this case is unaffected by the patch. gevent's code literally looks like this:

import signal as _signal
_signal_signal = _signal.signal # capture before patch
def signal(signalnum, handler):
    """
    Exactly the same as :func:`signal.signal` except where
    :const:`signal.SIGCHLD` is concerned.
    """
    if signalnum != _signal.SIGCHLD:
        return _signal_signal(signalnum, handler)
   ...
Contributor

jamadden commented Oct 19, 2015

Yes, there is a monkey patch on signal.signal in 1.1, but it only handles the case where the first argument is SIGCHLD. Everything else is passed through to the standard library, including SIGQUIT and SIGINT. So this case is unaffected by the patch. gevent's code literally looks like this:

import signal as _signal
_signal_signal = _signal.signal # capture before patch
def signal(signalnum, handler):
    """
    Exactly the same as :func:`signal.signal` except where
    :const:`signal.SIGCHLD` is concerned.
    """
    if signalnum != _signal.SIGCHLD:
        return _signal_signal(signalnum, handler)
   ...
@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 19, 2015

Collaborator

Oh, right. You said that. Sorry.

Collaborator

tilgovi commented Oct 19, 2015

Oh, right. You said that. Sorry.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 19, 2015

Collaborator

I think this PR looks good enough to me.

Collaborator

tilgovi commented Oct 19, 2015

I think this PR looks good enough to me.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 26, 2015

Collaborator

I'm not hearing any objection. Pushing the button.

Collaborator

tilgovi commented Oct 26, 2015

I'm not hearing any objection. Pushing the button.

tilgovi added a commit that referenced this pull request Oct 26, 2015

Merge pull request #1128 from NextThought/1126-gevent-time
Make the gevent workers handle the quit signal by deferring to a new greenlet

@tilgovi tilgovi merged commit 821123d into benoitc:master Oct 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

simon-weber added a commit to simon-weber/gchatautorespond that referenced this pull request Sep 2, 2016

@ant1g

This comment has been minimized.

Show comment
Hide comment
@ant1g

ant1g Nov 17, 2017

Contributor

I am having the exact same issue, but for the handle_usr1... Shall create a ticket? The solution is going to be the same as for this issue. I can create PR if needed.

Contributor

ant1g commented Nov 17, 2017

I am having the exact same issue, but for the handle_usr1... Shall create a ticket? The solution is going to be the same as for this issue. I can create PR if needed.

@jamadden jamadden deleted the NextThought:1126-gevent-time branch Nov 17, 2017

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Nov 18, 2017

Collaborator

@ant1g go for it! Posting a traceback would be helpful, too, in case there's something else going on that someone can spot.

Collaborator

tilgovi commented Nov 18, 2017

@ant1g go for it! Posting a traceback would be helpful, too, in case there's something else going on that someone can spot.

@ant1g

This comment has been minimized.

Show comment
Hide comment
@ant1g

ant1g Nov 19, 2017

Contributor

OK, I have created: #1645

Contributor

ant1g commented Nov 19, 2017

OK, I have created: #1645

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

Merge pull request #1128 from NextThought/1126-gevent-time
Make the gevent workers handle the quit signal by deferring to a new greenlet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment