Set Connection: closed when running without request queueing
#2216
Conversation
This helps make it clearer how this branching works, and explicitly documents that this is a difference in which default HTTP 1.0 vs 1.1 assume.
When running with queue_requests=false, make the Connection header reflect that the server will close the connection after the request is completed. (This is required by https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html, specifically: "HTTP/1.1 applications that do not support persistent connections MUST include the "close" connection option in every message.") This could previously cause clients which looked at this header to attempt to reuse connections which the server had closed.
| @@ -1,4 +1,6 @@ | |||
| require_relative "helper" | |||
| require "puma/events" | |||
| require "net/http" | |||
There was a problem hiding this comment.
This is conceptually unrelated to the rest of the change, but this test file needs these includes to run individually.
| sock.close | ||
| end | ||
|
|
||
| def test_http11_connection_header_no_queue |
There was a problem hiding this comment.
This test fails before the bugfix - the other 3 worked before and are just to ensure no regressions / generally exercise this code.
|
Hrm - the builds seem to be flaking. (The builds are failing with timeouts, and don't seem to be failing in a consistent way.) Does anybody have any advice on what to do here? |
|
I've seen this before, but not as bad as it is now. Before, it was an occasional job in a group of several. Now, sometimes it's most of the Ubuntu jobs. I wish I knew where to file an issue for it... |
|
Oof - the latest build failure is an error from inside of Ragel, from a different test: https://github.com/puma/puma/pull/2216/checks?check_run_id=557243347 |
|
Great! All builds pass - so it seems like I was just getting unlucky. (This issue looks maybe related #2148, but honestly I don't really know.) I might try taking a swing at debugging what's going on here - but for now, this PR is ready for review. |
|
Thanks! Good work. |
Description
When running with
queue_requests=false, the server closes the connection after each request. However, the server would indicate via theConnectionheader that the connection would be persisted. This was happening because the headers got reported inhandle_request, before the check to@queue_requestsinprocess_clientforces the connection to be closed. This PR moves the check to be inhandle_request- this ensures that what the server reports in the headers, and what it actually does with the connection can't get out of whack.Specifically the behaviour that we want here is:
ConnectionheaderConnection: closed. This is required by https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html, specifically:HTTP/1.1 applications that do not support persistent connections MUST include the "close" connection option in every message.As part of doing this change, I've refactored the logic around setting the connection headers to make it easier to understand why the headers are set the way they are. I've pulled this into a separate commit (345e127, which is intended to purely be a refactor and to have no functional effect). In general, this PR is probably best reviewed commit-by-commit.
The motivation for making this change is that this could previously cause clients which looked at this header to attempt to reuse connections which the server had closed. Clients that do this would get sent a TCP RST packet for their troubles, and would raise an exception.
Your checklist for this pull request
[changelog skip]the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.