Skip to content

Commit

Permalink
let's exception not bubble
Browse files Browse the repository at this point in the history 
ensure we can catch  correctly  exceptions based on BaseException.

Note: patch was origninally proposed by the pr #2923, but original
author closed it.

Fix #2923
  • Loading branch information
@benoitc
benoitc committed Dec 7, 2023
1 parent ca9162d commit 4023228
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion gunicorn/workers/base_async.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def handle(self, listener, client, addr):
self.log.debug("Ignoring socket not connected")
else:
self.log.debug("Ignoring EPIPE")
except Exception as e:
except BaseException as e:
self.handle_error(req, client, addr, e)
finally:
util.close(client)
Expand Down
2 changes: 1 addition & 1 deletion gunicorn/workers/sync.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ def handle(self, listener, client, addr):
self.log.debug("Ignoring socket not connected")
else:
self.log.debug("Ignoring EPIPE")
except Exception as e:
except BaseException as e:

This comment has been minimized.

Sign in to view

Copy link
@fsrajer

fsrajer Jul 24, 2024

Hello @benoitc . We are happy users of gunicorn and deploy it in scale in our app. I just wanted to mention that this change is catching even the SystemExit errors, which are triggered by sys.exit. These sys.exit are called in methods like handle_quit or handle_abort, which are typically already logged as "WORKER TIMEOUT". Not sure if this was intended by the change. It is causing us a bit of a hurdle in monitoring, so I just wanted to call it out.

This comment has been minimized.

Sign in to view

Copy link
@pajod

pajod Jul 24, 2024

Contributor

Context:
#2923
#3207
I strongly believe gevent-specific stuff should be handled strictly scoped to those gevent special cases. There is no good reason to have that make other workers more complicated or behave unexpectedly.

This comment has been minimized.

Sign in to view

Copy link
@fsrajer

fsrajer Jul 25, 2024

Your comment made me look into the codebase again. I think that the change from Exception to BaseException here is a bug actually.

The intention of handle_quit is to shutdown the worker, but with BaseException catching, the sys.exit does nothing. The worker goes out of scope eventually due to alive=False, but then it doesn't make sense to call sys.exit.

I would propose to revert catching back to Exception.

This comment has been minimized.

Sign in to view

Copy link
@benoitc

benoitc Jul 25, 2024

Author Owner

@fsrajer what steps are needed to reproduce? Do you reproduce it only with the gevent worker or is this also reproducible with other aync workers?

This comment has been minimized.

Sign in to view

Copy link
@fsrajer

fsrajer Jul 25, 2024

@benoitc , I didn't look into any other than sync worker.

This is my observation sequentially that shows the issue:

  • base Worker links handle_quit to SIGQUIT, SIGINT and similarly handle_abort to SIGABRT
    • both methods trigger SystemExit by sys.exit to shutdown, which derives from BaseException
  • arbiter or someone else wants to explicitly shutdown the worker (e.g. with SIGABRT on timeout)
  • SyncWorker catches BaseException in handle
    • and instead of shutting down right away, it continues to the next iteration and stops gracefully via self.alive
self.handle_error(req, client, addr, e)
finally:
util.close(client)
Expand Down

0 comments on commit 4023228

Please sign in to comment.