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

handle HaltServer in manage_workers #1095

Merged
merged 1 commit into from Dec 26, 2015

Conversation

Projects
None yet
3 participants
@danc86
Contributor

danc86 commented Aug 11, 2015

In Arbiter#run() the call to self.manage_workers() can also raise HaltServer, but it's outside the try block where it is handled.

For example (from RHBZ#1200041):


Traceback (most recent call last):
  File "/usr/bin/gunicorn", line 9, in <module>
    load_entry_point('gunicorn==19.1.1', 'console_scripts', 'gunicorn')()
  File "/usr/lib/python2.7/site-packages/gunicorn/app/wsgiapp.py", line 74, in run
    WSGIApplication("%(prog)s [OPTIONS] [APP_MODULE]").run()
  File "/usr/lib/python2.7/site-packages/gunicorn/app/base.py", line 185, in run
    super(Application, self).run()
  File "/usr/lib/python2.7/site-packages/gunicorn/app/base.py", line 71, in run
    Arbiter(self).run()
  File "/usr/lib/python2.7/site-packages/gunicorn/arbiter.py", line 169, in run
    self.manage_workers()
  File "/usr/lib/python2.7/site-packages/gunicorn/arbiter.py", line 477, in manage_workers
    self.spawn_workers()
  File "/usr/lib/python2.7/site-packages/gunicorn/arbiter.py", line 542, in spawn_workers
    time.sleep(0.1 * random.random())
  File "/usr/lib/python2.7/site-packages/gunicorn/arbiter.py", line 209, in handle_chld
    self.reap_workers()
  File "/usr/lib/python2.7/site-packages/gunicorn/arbiter.py", line 459, in reap_workers
    raise HaltServer(reason, self.WORKER_BOOT_ERROR)
HaltServer: <HaltServer 'Worker failed to boot.' 3>
@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Aug 11, 2015

Collaborator

This error, a worker failing to boot, may be intentionally considered fatal.

Collaborator

tilgovi commented Aug 11, 2015

This error, a worker failing to boot, may be intentionally considered fatal.

@danc86

This comment has been minimized.

Show comment
Hide comment
@danc86

danc86 Aug 11, 2015

Contributor

Right, I would definitely expect gunicorn to terminate with an error message. The issue is just that, right now, the exception is sometimes unhandled, which on Fedora causes abrt to file a bug with the traceback... whereas self.halt() should really be called. And you can see, there is an except: block which tries to do that, it just doesn't cover self.manage_workers() I guess because the possibility of the SIGCHLD handler happening inside manage_workers was not considered.

Contributor

danc86 commented Aug 11, 2015

Right, I would definitely expect gunicorn to terminate with an error message. The issue is just that, right now, the exception is sometimes unhandled, which on Fedora causes abrt to file a bug with the traceback... whereas self.halt() should really be called. And you can see, there is an except: block which tries to do that, it just doesn't cover self.manage_workers() I guess because the possibility of the SIGCHLD handler happening inside manage_workers was not considered.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Aug 11, 2015

Collaborator

Okay, thanks for that additional detail. Cheers.

Collaborator

tilgovi commented Aug 11, 2015

Okay, thanks for that additional detail. Cheers.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 23, 2015

Owner

+1 to merge it

Owner

benoitc commented Dec 23, 2015

+1 to merge it

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

@tilgovi tilgovi merged commit 2ac41c4 into benoitc:master Dec 26, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 26, 2015

Collaborator

Merged. Thanks, @danc86.

Collaborator

tilgovi commented Dec 26, 2015

Merged. Thanks, @danc86.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment