Set Connection: closed when running without request queueing (#2216)

* Fix test requires

* Refactor how the HTTP connection header is set

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.

* Fix Connection header when not using queue_requests

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.

* History

* Re-roll CI builds

* Rebuild again

History.md

@@ -27,6 +27,7 @@
   * Ensure `BUNDLE_GEMFILE` is unspecified in workers if unspecified in master when using `prune_bundler` (#2154)
   * Rescue and log exceptions in hooks defined by users (on_worker_boot, after_worker_fork etc) (#1551)
   * Read directly from the socket in #read_and_drop to avoid raising further SSL errors (#2198)
+  * Set `Connection: closed` header when queue requests is disabled (#2216)
 
 * Refactor
   * Remove unused loader argument from Plugin initializer (#2095)

lib/puma/server.rb

@@ -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
@@ -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
@@ -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
           #
@@ -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|
@@ -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

test/test_puma_server.rb

@@ -1,4 +1,6 @@
 require_relative "helper"
+require "puma/events"
+require "net/http"
 
 class TestPumaServer < Minitest::Test
   parallelize_me!
@@ -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
+    @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