From ff94412c41504ad2790293068bf3efd3abd2c0e0 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 Mar 2025 00:21:32 +1300 Subject: [PATCH 1/2] Persistent can only transition from `true` to `false`. --- lib/protocol/http1/connection.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/protocol/http1/connection.rb b/lib/protocol/http1/connection.rb index ca81618..63707d4 100644 --- a/lib/protocol/http1/connection.rb +++ b/lib/protocol/http1/connection.rb @@ -320,7 +320,11 @@ def read_request headers = read_headers - @persistent = persistent?(version, method, headers) + # If we are not persistent, we can't become persistent even if the request might allow it: + if @persistent + # In other words, `@persistent` can only transition from true to false. + @persistent = persistent?(version, method, headers) + end body = read_request_body(method, headers) From 08c794c5c969594491c11ee5736b3753b16f40a7 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 13 Mar 2025 13:26:43 +1300 Subject: [PATCH 2/2] Add more tests for persistent connection handling. --- lib/protocol/http1/connection.rb | 6 +- test/protocol/http1/connection.rb | 8 ++ test/protocol/http1/connection/persistent.rb | 133 +++++++++++++++++++ 3 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 test/protocol/http1/connection/persistent.rb diff --git a/lib/protocol/http1/connection.rb b/lib/protocol/http1/connection.rb index 63707d4..b13b52d 100644 --- a/lib/protocol/http1/connection.rb +++ b/lib/protocol/http1/connection.rb @@ -148,7 +148,7 @@ def persistent?(version, method, headers) else return false end - else + else # HTTP/1.1+ if connection = headers[CONNECTION] return !connection.close? else @@ -362,7 +362,9 @@ def read_response(method) headers = read_headers - @persistent = persistent?(version, method, headers) + if @persistent + @persistent = persistent?(version, method, headers) + end unless interim_status?(status) body = read_response_body(method, status, headers) diff --git a/test/protocol/http1/connection.rb b/test/protocol/http1/connection.rb index cc9d690..f88c69f 100644 --- a/test/protocol/http1/connection.rb +++ b/test/protocol/http1/connection.rb @@ -30,6 +30,8 @@ expect(version).to be == "HTTP/1.1" expect(headers).to be == {} expect(body).to be_nil + + expect(server).to be(:persistent) end it "reads request without body after closing connection" do @@ -44,6 +46,8 @@ expect(version).to be == "HTTP/1.1" expect(headers).to be == {"accept" => ["*/*"], "header-0" => ["value 1"]} expect(body).to be_nil + + expect(server).to be(:persistent) end it "reads request with fixed body" do @@ -58,6 +62,8 @@ expect(version).to be == "HTTP/1.1" expect(headers).to be == {} expect(body.join).to be == "Hello World" + + expect(server).to be(:persistent) end it "reads request with chunked body" do @@ -72,7 +78,9 @@ expect(version).to be == "HTTP/1.1" expect(headers).to be == {} expect(body.join).to be == "Hello World" + expect(server).to be(:persistent?, version, method, headers) + expect(server).to be(:persistent) end it "reads request with CONNECT method" do diff --git a/test/protocol/http1/connection/persistent.rb b/test/protocol/http1/connection/persistent.rb new file mode 100644 index 0000000..7644579 --- /dev/null +++ b/test/protocol/http1/connection/persistent.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +# Released under the MIT License. +# Copyright, 2025, by Samuel Williams. + +require "protocol/http1/connection" +require "protocol/http/body/buffered" +require "protocol/http/body/writable" + +require "connection_context" + +describe Protocol::HTTP1::Connection do + include_context ConnectionContext + + with "multiple requests in a single connection" do + it "handles two back-to-back GET requests (HTTP/1.1 keep-alive)" do + client.write_request("localhost", "GET", "/first", "HTTP/1.1", {"Header-A" => "Value-A"}) + client.write_body("HTTP/1.1", nil) + expect(client).to be(:half_closed_local?) + + # Server reads it: + authority, method, path, version, headers, body = server.read_request + expect(authority).to be == "localhost" + expect(method).to be == "GET" + expect(path).to be == "/first" + expect(version).to be == "HTTP/1.1" + expect(headers["header-a"]).to be == ["Value-A"] + expect(body).to be_nil + + # Server writes a response: + expect(server).to be(:half_closed_remote?) + server.write_response("HTTP/1.1", 200, {"Res-A" => "ValA"}, "OK") + server.write_body("HTTP/1.1", nil) + expect(server).to be(:idle?) + + # Client reads first response: + version, status, reason, headers, body = client.read_response("GET") + expect(version).to be == "HTTP/1.1" + expect(status).to be == 200 + expect(reason).to be == "OK" + expect(headers["res-a"]).to be == ["ValA"] + expect(body).to be_nil + + # Now both sides should be back to :idle (persistent re-use): + expect(client).to be(:idle?) + expect(server).to be(:idle?) + + # Second request: + client.write_request("localhost", "GET", "/second", "HTTP/1.1", {"Header-B" => "Value-B"}) + client.write_body("HTTP/1.1", nil) + expect(client).to be(:half_closed_local?) + + # Server reads it: + authority, method, path, version, headers, body = server.read_request + expect(authority).to be == "localhost" + expect(method).to be == "GET" + expect(path).to be == "/second" + expect(version).to be == "HTTP/1.1" + expect(headers["header-b"]).to be == ["Value-B"] + expect(body).to be_nil + + # Server writes a response: + expect(server).to be(:half_closed_remote?) + server.write_response("HTTP/1.1", 200, {"Res-B" => "ValB"}, "OK") + server.write_body("HTTP/1.1", nil) + + # Client reads second response: + version, status, reason, headers, body = client.read_response("GET") + expect(version).to be == "HTTP/1.1" + expect(status).to be == 200 + expect(reason).to be == "OK" + expect(headers["res-b"]).to be == ["ValB"] + expect(body).to be_nil + + # Confirm final states: + expect(client).to be(:idle?) + expect(server).to be(:idle?) + end + end + + with "partial body read" do + it "closes correctly if server does not consume entire fixed-length body" do + # Indicate Content-Length = 11 but only read part of it on server side: + client.stream.write "POST / HTTP/1.1\r\nHost: localhost\r\nContent-Length: 11\r\n\r\nHello" + client.stream.close + + # Server reads request line/headers: + authority, method, path, version, headers, body = server.read_request + expect(method).to be == "POST" + expect(body).to be_a(Protocol::HTTP1::Body::Fixed) + + # Partially read 5 bytes only: + partial = body.read + expect(partial).to be == "Hello" + + expect do + body.read + end.to raise_exception(EOFError) + + # Then server forcibly closes read (simulating a deliberate stop): + server.close_read + + # Because of partial consumption, that should move the state to half-closed remote or closed, etc. + expect(server).to be(:half_closed_remote?) + expect(server).not.to be(:persistent) + end + end + + with "no persistence" do + it "closes connection after request" do + server.persistent = false + + client.write_request("localhost", "GET", "/first", "HTTP/1.1", {"Header-A" => "Value-A"}) + client.write_body("HTTP/1.1", nil) + expect(client).to be(:half_closed_local?) + + # Server reads it: + authority, method, path, version, headers, body = server.read_request + expect(authority).to be == "localhost" + expect(method).to be == "GET" + expect(path).to be == "/first" + expect(version).to be == "HTTP/1.1" + expect(headers["header-a"]).to be == ["Value-A"] + expect(body).to be_nil + + # Server writes a response: + expect(server).to be(:half_closed_remote?) + server.write_response("HTTP/1.1", 200, {"Res-A" => "ValA"}, "OK") + server.write_body("HTTP/1.1", nil) + expect(server).to be(:closed?) + end + end +end