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

wsgi: Handle Timeouts from applications #911

Merged
merged 1 commit into from
Feb 6, 2024
Merged

wsgi: Handle Timeouts from applications #911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion eventlet/wsgi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

@4383 4383 Feb 5, 2024 •
edited

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

Copy link
Member

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.

[1] https://github.com/eventlet/eventlet/blob/799dabcb3fffb81a15f1b9fb1930e4e28edd4a12/eventlet/timeout.py#L34C1-L35C22

Copy link
Member

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?

Copy link
Contributor Author

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

  1. bugs happen and

  2. 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 a Transfer-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.

Copy link
Member

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.

self.close_connection = 1
tb = traceback.format_exc()
self.server.log.info(tb)
Expand Down
41 changes: 41 additions & 0 deletions tests/wsgi_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1570,6 +1570,47 @@ def wsgi_app(environ, start_response):
self.assertEqual(result.status, 'HTTP/1.1 500 Internal Server Error')
self.assertEqual(result.headers_lower['connection'], 'close')
assert 'transfer-encoding' not in result.headers_lower
assert 'Traceback' in self.logfile.getvalue()
assert 'RuntimeError: intentional error' in self.logfile.getvalue()

def test_timeouts_in_app_call(self):
def wsgi_app(environ, start_response):
start_response('200 OK', [('Content-Type', 'text/plain')])
yield b'partial '
raise eventlet.Timeout()
yield b'body\n'
self.site.application = wsgi_app
sock = eventlet.connect(self.server_addr)
sock.sendall(b'GET / HTTP/1.1\r\nHost: localhost\r\n\r\n')
result = read_http(sock)
self.assertEqual(result.status, 'HTTP/1.1 500 Internal Server Error')
self.assertEqual(result.headers_lower['connection'], 'close')
assert 'transfer-encoding' not in result.headers_lower
assert 'content-length' in result.headers_lower
assert 'Traceback' in self.logfile.getvalue()
assert 'Timeout' in self.logfile.getvalue()

def test_timeouts_in_app_iter(self):
def wsgi_app(environ, start_response):
environ['eventlet.minimum_write_chunk_size'] = 1
start_response('200 OK', [('Content-Type', 'text/plain'),
('Content-Length', '13')])

def app_iter():
yield b'partial '
raise eventlet.Timeout()
yield b'body\n'
return app_iter()
self.site.application = wsgi_app
sock = eventlet.connect(self.server_addr)
sock.sendall(b'GET / HTTP/1.1\r\nHost: localhost\r\n\r\n')
result = read_http(sock)
self.assertEqual(result.status, 'HTTP/1.1 200 OK')
assert 'connection' not in result.headers_lower
self.assertEqual(result.headers_lower['content-length'], '13')
self.assertEqual(len(result.body), 8)
assert 'Traceback' in self.logfile.getvalue()
assert 'Timeout' in self.logfile.getvalue()

def test_unicode_with_only_ascii_characters_works(self):
def wsgi_app(environ, start_response):
Expand Down