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.

Sign up for GitHub

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

Jump to bottom

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

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

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

wants to merge 4 commits into from

Conversation

pajod
Copy link
Contributor

@pajod pajod commented Aug 14, 2024 •
edited
Loading

The report in #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 #3207

Context:

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:

    gunicorn/gunicorn/workers/geventlet.py

    Lines 153 to 156 in 903792f

    def handle(self, listener, client, addr):
    if self.cfg.is_ssl:
    client = ssl_wrap_socket(client, self.cfg)
    super().handle(listener, client, addr)

@pajod
Revert "let's exception not bubble"
1fcadcb 
This reverts commit 4023228.

We use sys.exit. On purpose.
We should therefore not be catching SystemExit.
@pajod
workaround: reintroduce gevent.Timeout handling
8617c39 
This is probably wrong. But less wrong than handling *all*
BaseException. Reports of this happening may be the result of
 some *other* async Timeout in the wsgi app bubbling through to us.

Gevent docs promise:
"[..] if *exception* is the literal ``False``, the timeout is still raised,
   but the context manager suppresses it, so
   the code outside the with-block won't see it."
@pajod
style: line break
ef94875
@pajod pajod mentioned this pull request Aug 14, 2024
Open
@benoitc benoitc self-assigned this Aug 26, 2024
@pajod
Copy link
Contributor Author

pajod commented Aug 27, 2024

@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: pajod#3)

@benoitc
Copy link
Owner

benoitc commented Sep 1, 2024

@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?

@pajod
Partially revert "unconditionally log request error"
27be287 
There is not need to excessively produce high-severity logs, while
there are ways to reach that point from entirely benign network errors.

This partially reverts commit 0b10cba.
@pajod
Copy link
Contributor Author

pajod commented Sep 5, 2024

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

@pajod pajod mentioned this pull request Sep 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@benoitc benoitc

Labels
working on it :)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error handling request (no URI read)
2 participants
@pajod @benoitc