Fix rack.after_reply exceptions breaking connections #2861
Conversation
Currently, when a `rack.after_reply` callable raises an exception we attempt to handle it like other client errors by responding with an HTTP 500 response. This however doesn't work because `rack.after_reply` callbacks are called after the response body has already been written to the client. This can cause issues with re-used connections. This is because 2 HTTP responses are being returned for a single request. If a second HTTP request is made before the error handling logic completes the timing can line up causing the second HTTP response to be served a 500 from the first HTTP requests `rack.after_reply` callbacks raising. That may look roughly like: 1. Request 1 starts, opening a reusable TCP connection 2. Request 1 is written to and "completed" 3. Request 1 `rack.after_reply` callables are called 4. Request 2 starts, reusing the same TCP connection as request 1 5. `rack.after_reply` raises, calls `client_error` and serves a 500 response 6. Request 2 receives the 500 response. This is somewhat difficult to reproduce using HTTP clients since it's a race condition whether or not the 500 is written at the "correct" time or not. To prevent this issue the `rack.after_reply` callables are now wrapped in a begin/rescue/end block that rescues from `StandardError` and logs instead of attempting to serve a 500 response.
test/test_rack_server.rb
Outdated
| # * If we would block trying to read from the socket, we can assume that | ||
| # the erroneous 500 response wasn't/won't be written. | ||
| sleep 0.1 | ||
| assert_raises IO::EAGAINWaitReadable do |
There was a problem hiding this comment.
I'm not the biggest fan of this test, but using Net::HTTP was flaky and the problem is pretty low-level so this is what I ended up with. It is technically testable using Net::HTTP but you have to instance_variable_get(:@socket) on the connection and I wasn't a huge fan of diving into stdlib internals.
Happy to get feedback/suggestions on alternatives here if anyone has any.
|
Now this is a high-quality first time contribution to a project! 🥇 |
|
Looks good to me but I'll leave it open for a bit to see if anyone else has anything to say |
|
This passes for me locally. The one thing not sure about is whether the begin/rescue block should be inside or outside the |
Same, I'm wondering if it's platform differences since most of the failures are on Windows. I'll push up what I think may fix it in a bit. I chose to put the rescue outside of the |
|
It looks like d044ed2 fixed the failures for the windows builds but all Ruby HEAD builds are still failing. I ran the suite against master in my fork too and it looks like Ruby HEAD is failing there too (my fork is 1 commit behind though, if that matters). |
|
Ignore the Puma may need some changes for issues related to default gems... EDIT: Most of the std-lib has been 'gemified', and testing against Ruby master in the actual gem repos has also caused issues. |
|
Also, OS socket errors may differ between Ubuntu, macOS, and Windows. Kind of a pita when one is wondering why test failures vary... |
* Fix rack.after_reply exceptions breaking connections Currently, when a `rack.after_reply` callable raises an exception we attempt to handle it like other client errors by responding with an HTTP 500 response. This however doesn't work because `rack.after_reply` callbacks are called after the response body has already been written to the client. This can cause issues with re-used connections. This is because 2 HTTP responses are being returned for a single request. If a second HTTP request is made before the error handling logic completes the timing can line up causing the second HTTP response to be served a 500 from the first HTTP requests `rack.after_reply` callbacks raising. That may look roughly like: 1. Request 1 starts, opening a reusable TCP connection 2. Request 1 is written to and "completed" 3. Request 1 `rack.after_reply` callables are called 4. Request 2 starts, reusing the same TCP connection as request 1 5. `rack.after_reply` raises, calls `client_error` and serves a 500 response 6. Request 2 receives the 500 response. This is somewhat difficult to reproduce using HTTP clients since it's a race condition whether or not the 500 is written at the "correct" time or not. To prevent this issue the `rack.after_reply` callables are now wrapped in a begin/rescue/end block that rescues from `StandardError` and logs instead of attempting to serve a 500 response. * Assert against less specific exception
* Fix rack.after_reply exceptions breaking connections Currently, when a `rack.after_reply` callable raises an exception we attempt to handle it like other client errors by responding with an HTTP 500 response. This however doesn't work because `rack.after_reply` callbacks are called after the response body has already been written to the client. This can cause issues with re-used connections. This is because 2 HTTP responses are being returned for a single request. If a second HTTP request is made before the error handling logic completes the timing can line up causing the second HTTP response to be served a 500 from the first HTTP requests `rack.after_reply` callbacks raising. That may look roughly like: 1. Request 1 starts, opening a reusable TCP connection 2. Request 1 is written to and "completed" 3. Request 1 `rack.after_reply` callables are called 4. Request 2 starts, reusing the same TCP connection as request 1 5. `rack.after_reply` raises, calls `client_error` and serves a 500 response 6. Request 2 receives the 500 response. This is somewhat difficult to reproduce using HTTP clients since it's a race condition whether or not the 500 is written at the "correct" time or not. To prevent this issue the `rack.after_reply` callables are now wrapped in a begin/rescue/end block that rescues from `StandardError` and logs instead of attempting to serve a 500 response. * Assert against less specific exception
Description
Currently, when a
rack.after_replycallable raises an exception weattempt to handle it like other client errors by responding with an HTTP
500 response. This however doesn't work because
rack.after_replycallbacks are called after the response body has already been written to
the client.
This can cause issues with re-used connections. This is because 2 HTTP
responses are being returned for a single request. If a second HTTP
request is made before the error handling logic completes the timing can
line up causing the second HTTP response to be served a 500 from the
first HTTP requests
rack.after_replycallbacks raising.That may look roughly like:
rack.after_replycallables are calledrack.after_replyraises, callsclient_errorand serves a 500response
This is somewhat difficult to reproduce using HTTP clients since it's a
race condition whether or not the 500 is written at the "correct" time
or not.
To prevent this issue the
rack.after_replycallables are now wrappedin a begin/rescue/end block that rescues from
StandardErrorand logsinstead of attempting to serve a 500 response.
Closes #2856
Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.