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
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
25 changes: 15 additions & 10 deletions lib/puma/server.rb
Expand Up @@ -371,7 +371,6 @@ def process_client(client, buffer)
close_socket = false
return
when true
return unless @queue_requests
buffer.reset

ThreadPool.clean_thread_locals if clean_thread_locals
Expand Down Expand Up @@ -612,10 +611,10 @@ def handle_request(req, lines)
line_ending = LINE_END
colon = COLON

http_11 = if env[HTTP_VERSION] == HTTP_11
http_11 = env[HTTP_VERSION] == HTTP_11
if http_11
allow_chunked = true
keep_alive = env.fetch(HTTP_CONNECTION, "").downcase != CLOSE
include_keepalive_header = false

# An optimization. The most common response is 200, so we can
# reply with the proper 200 status without having to compute
Expand All @@ -629,11 +628,9 @@ def handle_request(req, lines)

no_body ||= status < 200 || STATUS_WITH_NO_ENTITY_BODY[status]
end
true
else
allow_chunked = false
keep_alive = env.fetch(HTTP_CONNECTION, "").downcase == KEEP_ALIVE
include_keepalive_header = keep_alive

# Same optimization as above for HTTP/1.1
#
Expand All @@ -645,9 +642,12 @@ def handle_request(req, lines)

no_body ||= status < 200 || STATUS_WITH_NO_ENTITY_BODY[status]
end
false
end

# regardless of what the client wants, we always close the connection
# if running without request queueing
keep_alive &&= @queue_requests

response_hijack = nil

headers.each do |k, vs|
Expand All @@ -674,10 +674,15 @@ def handle_request(req, lines)
end
end

if include_keepalive_header
lines << CONNECTION_KEEP_ALIVE
elsif http_11 && !keep_alive
lines << CONNECTION_CLOSE
# HTTP/1.1 & 1.0 assume different defaults:
# - HTTP 1.0 assumes the connection will be closed if not specified
# - HTTP 1.1 assumes the connection will be kept alive if not specified.
# Only set the header if we're doing something which is not the default
# for this protocol version
if http_11
lines << CONNECTION_CLOSE if !keep_alive
else
lines << CONNECTION_KEEP_ALIVE if keep_alive
end

if no_body
Expand Down
41 changes: 41 additions & 0 deletions test/test_puma_server.rb
@@ -1,4 +1,6 @@
require_relative "helper"
require "puma/events"
require "net/http"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is conceptually unrelated to the rest of the change, but this test file needs these includes to run individually.


class TestPumaServer < Minitest::Test
parallelize_me!
Expand Down Expand Up @@ -880,4 +882,43 @@ def assert_does_not_allow_http_injection(app, opts = {})
assert_does_not_allow_http_injection(app)
end
end

def test_http11_connection_header_queue
server_run app: ->(_) { [200, {}, [""]] }

sock = send_http "GET / HTTP/1.1\r\n\r\n"
assert_equal ["HTTP/1.1 200 OK", "Content-Length: 0"], header(sock)

sock << "GET / HTTP/1.1\r\nConnection: close\r\n\r\n"
assert_equal ["HTTP/1.1 200 OK", "Connection: close", "Content-Length: 0"], header(sock)

sock.close
end

def test_http10_connection_header_queue
server_run app: ->(_) { [200, {}, [""]] }

sock = send_http "GET / HTTP/1.0\r\nConnection: keep-alive\r\n\r\n"
assert_equal ["HTTP/1.0 200 OK", "Connection: Keep-Alive", "Content-Length: 0"], header(sock)

sock << "GET / HTTP/1.0\r\n\r\n"
assert_equal ["HTTP/1.0 200 OK", "Content-Length: 0"], header(sock)
sock.close
end

def test_http11_connection_header_no_queue
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@server = Puma::Server.new @app, @events, queue_requests: false
server_run app: ->(_) { [200, {}, [""]] }
sock = send_http "GET / HTTP/1.1\r\n\r\n"
assert_equal ["HTTP/1.1 200 OK", "Connection: close", "Content-Length: 0"], header(sock)
sock.close
end

def test_http10_connection_header_no_queue
@server = Puma::Server.new @app, @events, queue_requests: false
server_run app: ->(_) { [200, {}, [""]] }
sock = send_http "GET / HTTP/1.0\r\n\r\n"
assert_equal ["HTTP/1.0 200 OK", "Content-Length: 0"], header(sock)
sock.close
end
end