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.

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

Set `Connection: closed` when running without request queueing #2216

Merged
merged 6 commits into from Apr 6, 2020

Conversation

@praboud-stripe
Copy link
Contributor

praboud-stripe commented Apr 3, 2020

Description

When running with queue_requests=false, the server closes the connection after each request. However, the server would indicate via the Connection header that the connection would be persisted. This was happening because the headers got reported in handle_request, before the check to @queue_requests in process_client forces the connection to be closed. This PR moves the check to be in handle_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:

  • for HTTP/1.0, omit the Connection header
  • for HTTP/1.1, setting Connection: 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

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.
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"

This comment has been minimized.

Copy link
@praboud-stripe

praboud-stripe Apr 3, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@praboud-stripe

praboud-stripe Apr 3, 2020

Author Contributor

This test fails before the bugfix - the other 3 worked before and are just to ensure no regressions / generally exercise this code.

@praboud-stripe praboud-stripe force-pushed the stripe:praboud-connection branch from be61e2d to 8901a77 Apr 3, 2020
@praboud-stripe
Copy link
Contributor Author

praboud-stripe commented Apr 3, 2020

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?

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Apr 3, 2020

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...

@praboud-stripe
Copy link
Contributor Author

praboud-stripe commented Apr 3, 2020

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
I don't know if I'm just getting unlucky, or if there's actually a persistent failure on the Ubuntu 2.2 build.

@praboud-stripe
Copy link
Contributor Author

praboud-stripe commented Apr 3, 2020

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.

@nateberkopec
Copy link
Member

nateberkopec commented Apr 6, 2020

Thanks! Good work.

@nateberkopec nateberkopec merged commit 7316dbd into puma:master Apr 6, 2020
27 checks passed
27 checks passed
build
Details
ubuntu-18.04, 2.2
Details
ubuntu-18.04, 2.3
Details
ubuntu-18.04, 2.4
Details
ubuntu-18.04, 2.5
Details
ubuntu-18.04, 2.6
Details
ubuntu-18.04, 2.7
Details
ubuntu-18.04, head
Details
ubuntu-18.04, jruby
Details
ubuntu-18.04, truffleruby-head
Details
macos, 2.2
Details
macos, 2.3
Details
macos, 2.4
Details
macos, 2.5
Details
macos, 2.6
Details
macos, 2.7
Details
macos, head
Details
macos, jruby
Details
macos, truffleruby-head
Details
windows-latest, 2.2
Details
windows-latest, 2.3
Details
windows-latest, 2.4
Details
windows-latest, 2.5
Details
windows-latest, 2.6
Details
windows-latest, 2.7
Details
windows-latest, mingw
Details
ubuntu-latest, jruby-head ubuntu-latest, jruby-head
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.