-
Notifications
You must be signed in to change notification settings - Fork 320
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
wsgi: Handle Timeouts from applications #911
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #911 +/- ##
=====================================
Coverage 56% 56%
=====================================
Files 89 89
Lines 9722 9722
Branches 1810 1810
=====================================
+ Hits 5462 5463 +1
+ Misses 3885 3884 -1
Partials 375 375
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e301a0e
to
cc48298
Compare
@@ -631,7 +631,7 @@ def cap(x): | |||
write(b''.join(towrite)) | |||
if not headers_sent or (use_chunked[0] and just_written_size): | |||
write(b'') | |||
except Exception: | |||
except (Exception, eventlet.Timeout): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout
[1] inherit from BaseException
[2]. The Python documentation say that user defined exceptions should be inherited from Exception
[3]. What do you think of simply inherit from Exception
in Timeout
? We would not have to specifically handle timeouts like you propose here.
IMO the behavior you try to fix here is more a side effect of a bad practice in inheritance management. I'd simply suggest to fix that inheritance.
[1] https://github.com/eventlet/eventlet/blob/799dabcb3fffb81a15f1b9fb1930e4e28edd4a12/eventlet/timeout.py#L38C15-L38C28
[2] https://docs.python.org/3/library/exceptions.html#BaseException
[3] https://docs.python.org/3/library/exceptions.html#Exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment [1] in the timeout module say:
# deriving from BaseException so that "except Exception as e" doesn't catch
# Timeout exceptions.
So, apparently using BaseException
is something wanted by design... apparently the goal of using BaseException
is to leave it blow up...
So maybe either these changes are not something we should do, or, if you really think that timeouts shouldn't "blow up", then, we should move, IMO, from BaseException
to Exception
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May users want to know if Timeout happened, and may users want to implement retries logics with tools like tenacity
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind Timeout
inheriting from BaseException
is to ensure it cuts through any intermediary code that wasn't explicitly designed around eventlet, its timeouts, and its monkey-patching. So you can do something like
eventlet.monkey_patch(socket=True)
try:
with eventlet.Timeout(60):
resp = requests.get(...)
# read & process response
except eventlet.Timeout:
# handle time-limit-exceeded
and not worry about requests
(or urllib3
, or stdlib) having some generic except Exception:
handler that would swallow/translate/retry the exception. Instead, you get something approaching a hard cap on total request/response time (which, requests is careful to point out, is not how the timeout
in its API works).
Generally, developers should follow that sort of a pattern (try
/ with Timeout
/ except Timeout
) and not let the timeout escape, but
-
bugs happen and
-
when a timeout occurs in the app iter, the appropriate behavior is tricky.
Raising some sort of error is necessary -- otherwise we currently get a live-lock if the app provided a
Content-Length
but not enough bytes to satisfy it, or we mis-represent that aTransfer-Encoding: chunked
response was complete rather than truncated. And catching the timeout just to raise some other exception feels unnecessary, particularly when your WSGI server is the same project that gave you this useful tool!
As the WSGI server, the buck stops here. It's our job to ensure that the client gets a response (which it won't today, if the timeout escapes the app call), and it's way more obvious (at least, to me) that resources are properly cleaned up if we handle timeouts just like every other exception rather than let them continue all the way up to the hub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details. According for your latest comment, your changes LGTM.
If you're using eventlet as a web server, it's not unlikely that you'll be using eventlet.Timeouts at some point in your application callable or the response iterator that's returned. If they escape, don't let that blow up the whole worker greenthread, but treat it like other exceptions.
cc48298
to
c08915b
Compare
@@ -631,7 +631,7 @@ def cap(x): | |||
write(b''.join(towrite)) | |||
if not headers_sent or (use_chunked[0] and just_written_size): | |||
write(b'') | |||
except Exception: | |||
except (Exception, eventlet.Timeout): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the details. According for your latest comment, your changes LGTM.
If you're using eventlet as a web server, it's not unlikely that you'll be using
eventlet.Timeout
s at some point in your application callable or the response iterator that's returned. Don't let that blow up the whole worker greenthread, but treat it like other exceptions.