gunicorn icon indicating copy to clipboard operation
gunicorn copied to clipboard

revert: 4023228 ("let's exception not bubble")

Open pajod opened this issue 1 year ago • 5 comments

The report in https://github.com/benoitc/gunicorn/issues/2923 may have been not caused by a bug in Gunicorn or gevent in the first place. In any case, going from "this particular exception" to "each and every BaseException" was way excessive and is a likely explanation for https://github.com/benoitc/gunicorn/issues/3207

Context:

  • https://github.com/benoitc/gunicorn/commit/40232284934c32939c0e4e78caad1987c3773e08#r144628336

Suggested changes:

  • revert that patch
  • reintroduce a more specific exception catching that mimics what the reverted patch tried to accomplish, while emitting a new warning.

Unfinished thoughts:

  • the SSLError from wrap_socket in the eventlet worker (afaict, can only be triggered by the client sending garbage or disconnecting while Gunicorn was overloaded) cannot be dealt with this way due to class inheritance stepping outside the huge try-except block. I have a currently skipped test to reproduce this - the problem is over here: https://github.com/benoitc/gunicorn/blob/903792f152af6a27033d458020923cb2bcb11459/gunicorn/workers/geventlet.py#L153-L156
    • WIP: https://github.com/pajod/gunicorn/commit/f63030411982edf390149c8df5f10b8fdeaf8907

  • Partially reverts: 40232284934c32939c0e4e78caad1987c3773e08
  • Partially reverts: 0b10cbab1d6368fcab2d5a7b6fe359a6cecc81a7
  • Fixes: https://github.com/benoitc/gunicorn/issues/3207

pajod avatar Aug 14 '24 21:08 pajod

@benoitc Recommend you test this in conjunction with the two patches by sylt. With those, it is no longer a "simple" revert, but gets us closer to confirming the spurious tracebacks reported in #3207 are dealt with.

(hint: there is documentation on what exactly my integration tests currently encompass here: https://github.com/pajod/gunicorn/pull/3)

pajod avatar Aug 27 '24 15:08 pajod

@pajod PR must be provided over the master and don't assume that other PR may be added. There is no guarantee a separate PR will be acccepted, so PR will be enough by itself.

This PR sounds OK by itself. What could miss?

benoitc avatar Sep 01 '24 12:09 benoitc

@benoitc This PR fits on current master. It is OK to merge by itself.

pajod avatar Sep 05 '24 00:09 pajod

I found this PR through this issue: https://github.com/benoitc/gunicorn/issues/3207. @pajod are you still planning on merging this fix? Was it dropped in favor of a different solution? I'd love to see this fixed 😄

wholegrainloaf avatar Mar 07 '25 22:03 wholegrainloaf

@wholegrainloaf I still believe this revert should be done in general, and still believe a partial revert as suggested here - with a warning message - is the least intrusive way to go about it.

What is lacking is additional contributors to test & review this (any many more PRs). Ideally, someone would drop in and confirm having used the patch in a relevant (using those workers) environment, reporting back whether it is a clear improvement or not.

pajod avatar Mar 25 '25 23:03 pajod