From 675b8cb5d9ee939c1e2fbc7ed4e6a10997dfcd06 Mon Sep 17 00:00:00 2001 From: Denis Talakevich Date: Sun, 30 Aug 2026 15:52:01 +0300 Subject: [PATCH 1/6] Drain accepted streams after a graceful GOAWAY. Connection#receive_goaway closed the connection immediately, whatever the error code. For a graceful GOAWAY (error code 0) that is wrong: the peer will not accept new streams, but it is still processing the streams at or below last_stream_id and will send their responses (RFC 9113 6.8). Closing at once makes those requests fail with "Connection closed with N active stream(s)!" even though the peer processed them, which for a non-idempotent request cannot be retried. On a graceful GOAWAY the connection now records that it is going away, refuses and removes the streams above last_stream_id as before, and closes when the last accepted stream completes. A non-zero error code is unchanged. The state of the connection is decided before any stream's closed hook runs, so a hook which raises cannot leave it undecided and one which creates a stream cannot defeat the "nothing left to drain" check. create_stream refuses to open a locally-initiated stream once a GOAWAY has been received; streams the peer initiates are not covered by last_stream_id and stay legal. Connection#close detaches the active streams before closing them, so a re-entrant close cannot report a fabricated EOFError in place of the real error. Adds Connection#goaway_received? and Connection#draining?, which a client needs in order to stop offering the connection to new requests while it drains. --- lib/protocol/http2/connection.rb | 79 ++++++++++++++--- releases.md | 6 ++ test/protocol/http2/connection.rb | 136 +++++++++++++++++++++++++++++- 3 files changed, 209 insertions(+), 12 deletions(-) diff --git a/lib/protocol/http2/connection.rb b/lib/protocol/http2/connection.rb index e6f64de..96e019f 100644 --- a/lib/protocol/http2/connection.rb +++ b/lib/protocol/http2/connection.rb @@ -44,6 +44,9 @@ def initialize(framer, local_stream_id) @local_window = LocalWindow.new @remote_window = Window.new + + # Whether the remote peer has sent us a GOAWAY frame: + @goaway_received = false end # The connection stream ID (always 0 for connection-level operations). @@ -99,11 +102,39 @@ def closed? @state == :closed || @framer.nil? end + # Whether the remote peer has sent us a GOAWAY frame. We must not initiate any new streams on this connection, but the streams at or below `remote_stream_id` may still be in progress. + # @returns [Boolean] True if a GOAWAY frame has been received. + def goaway_received? + @goaway_received + end + + # Whether the connection is draining the streams which were accepted by the remote peer before it sent a graceful GOAWAY frame. Such a connection is still readable and those streams will still receive their responses, but it must not be used for new requests. + # @returns [Boolean] True if a graceful shutdown is in progress and streams are still active. + def draining? + @goaway_received && !self.closed? && @streams.any? + end + + # Transition the connection into the closed state if a graceful GOAWAY was received and there is nothing left to drain. + # + # As with {close!}, this is a state transition only: the owner of the connection is responsible for closing the underlying framer. + def close_if_drained! + if @goaway_received && @streams.empty? + self.close! + end + end + # Remove a stream from the active streams collection. + # + # If the remote peer has sent a graceful GOAWAY frame, the connection is only kept open in order to drain the streams it accepted, so when the last one completes there is nothing left to read. + # # @parameter id [Integer] The stream ID to remove. # @returns [Stream | Nil] The removed stream, or nil if not found. def delete(id) - @streams.delete(id) + stream = @streams.delete(id) + + self.close_if_drained! + + return stream end # Close the underlying framer and all streams. @@ -115,8 +146,10 @@ def close(error = nil) error = EOFError.new("Connection closed with #{@streams.size} active stream(s)!") end - @streams.each_value{|stream| stream.close(error)} - @streams.clear + # The streams are detached before any of them is closed, so that a re-entrant `close` - a stream's `closed` hook can reach one, e.g. by releasing the connection back to a pool - neither sees them as active nor closes them a second time with a different error: + streams, @streams = @streams, {} + + streams.each_value{|stream| stream.close(error)} ensure if @framer @@ -229,25 +262,36 @@ def send_goaway(error_code = 0, message = "") end # Process a GOAWAY frame from the remote peer. + # + # A GOAWAY frame with a zero error code is a graceful shutdown: the remote peer will not accept any new streams, but it is still processing the streams at or below `last_stream_id` and will send their responses (RFC 9113 §6.8). We must keep reading until those streams complete, otherwise requests which the remote peer has already processed - and whose side effects have already happened - fail locally. The connection is closed once the last of those streams completes, or the remote peer closes it. + # + # A GOAWAY frame with a non-zero error code is a connection error: the connection transitions into the closed state and {GoawayError} is raised. + # # @parameter frame [GoawayFrame] The GOAWAY frame to process. # @raises [GoawayError] If the frame indicates a connection error. def receive_goaway(frame) # We capture the last stream that was processed. @remote_stream_id, error_code, message = frame.unpack - self.close! + @goaway_received = true - # Streams above the last stream ID were not processed by the remote peer and are safe to retry (RFC 9113 §6.8). - error = ::Protocol::HTTP::RefusedError.new("GOAWAY: request not processed.") + # Streams above the last stream ID were not processed by the remote peer and are safe to retry (RFC 9113 §6.8). They are removed from the connection before being closed, both so that what remains is exactly the set of streams we are waiting on, and so that closing them cannot mutate the collection while we are traversing it. + refused_streams = @streams.select{|id, stream| id > @remote_stream_id} + refused_streams.each_key{|id| @streams.delete(id)} - @streams.each_value do |stream| - if stream.id > @remote_stream_id - stream.close(error) - end + # The state of the connection is decided before any stream is closed, so that it cannot be left undecided by a `closed` hook which raises, and cannot be influenced by one which creates a stream. + if error_code != 0 + self.close! + else + self.close_if_drained! + end + + unless refused_streams.empty? + error = ::Protocol::HTTP::RefusedError.new("GOAWAY: request not processed.") + refused_streams.each_value{|stream| stream.close(error)} end if error_code != 0 - # Shut down immediately. raise GoawayError.new(message, error_code) end end @@ -404,6 +448,14 @@ def valid_remote_stream_id?(stream_id) false end + # Check if the given stream ID represents a locally-initiated stream. + # This method should be overridden by client/server implementations. + # @parameter id [Integer] The stream ID to check. + # @returns [Boolean] True if the stream ID is locally-initiated. + def local_stream_id?(id) + false + end + # Accept an incoming stream from the other side of the connnection. # On the server side, we accept requests. def accept_stream(stream_id, &block) @@ -425,6 +477,11 @@ def accept_push_promise_stream(stream_id, &block) # On the client side, we create requests. # @return [Stream] the created stream. def create_stream(id = next_stream_id, &block) + if @goaway_received and local_stream_id?(id) + # Receivers of a GOAWAY frame MUST NOT open additional streams on the connection (RFC 9113 §6.8). A new connection has to be established for new streams. + raise ProtocolError, "Cannot create stream #{id} after GOAWAY!" + end + if @streams.key?(id) raise ProtocolError, "Cannot create stream with id #{id}, already exists!" end diff --git a/releases.md b/releases.md index 1e4f7bd..47d56f7 100644 --- a/releases.md +++ b/releases.md @@ -1,5 +1,11 @@ # Releases +## Unreleased + + - On a graceful `GOAWAY` (error code `0`), keep the connection open until the streams the remote peer accepted have completed, instead of closing it immediately and failing those requests with `EOFError`. + - `Connection#create_stream` refuses to open a locally-initiated stream once a `GOAWAY` has been received, as required by RFC 9113 §6.8. + - `Connection#close` detaches the active streams before closing them, so a re-entrant close cannot report a fabricated `EOFError` in place of the real error. + ## v0.26.2 - Ignore the reserved high bit when decoding GOAWAY last stream IDs. diff --git a/test/protocol/http2/connection.rb b/test/protocol/http2/connection.rb index 3f8b242..297339a 100644 --- a/test/protocol/http2/connection.rb +++ b/test/protocol/http2/connection.rb @@ -375,7 +375,11 @@ def before expect(client.read_frame).to be_a Protocol::HTTP2::GoawayFrame expect(client.remote_stream_id).to be == 1 - expect(client).to be(:closed?) + + # The server accepted stream 1 and is still processing it, so the connection is not closed yet: + expect(client).to be(:goaway_received?) + expect(client).to be(:draining?) + expect(client).not.to be(:closed?) # The server will ignore this frame as it was sent after the graceful shutdown: server.read_frame @@ -389,6 +393,10 @@ def before client.read_frame expect(stream.state).to be == :closed + + # There is nothing left to drain, so the connection is closed: + expect(client).not.to be(:draining?) + expect(client).to be(:closed?) end let(:stream_class) do @@ -442,6 +450,132 @@ def closed(error) # The processed stream (id=1) should still be open: expect(stream.state).not.to be == :closed + + # Unprocessed streams are removed from the connection, so what remains is exactly what we are waiting for: + expect(client.streams.keys).to be == [1] + end + + it "drains the streams which were accepted before a graceful GOAWAY" do + stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + + another_stream = client.create_stream do |connection, id| + stream_class.create(connection, id) + end + another_stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + + # Establish both request streams on the server: + server.read_frame + server.read_frame + + # The server accepted both streams, but will not accept any more: + server.send_goaway(0) + + client.read_frame + + expect(client).to be(:goaway_received?) + expect(client).to be(:draining?) + expect(client).not.to be(:closed?) + expect(client.streams.keys).to be == [1, 3] + + # Both streams still receive their response: + server.streams[1].send_headers(response_headers, Protocol::HTTP2::END_STREAM) + client.read_frame + + expect(stream.state).to be == :closed + expect(client).to be(:draining?) + expect(client).not.to be(:closed?) + + server.streams[3].send_headers(response_headers, Protocol::HTTP2::END_STREAM) + client.read_frame + + expect(another_stream.state).to be == :closed + expect(another_stream.error).to be_nil + + # The last accepted stream completed, so the connection is closed: + expect(client).not.to be(:draining?) + expect(client).to be(:closed?) + end + + it "refuses to create new streams after a graceful GOAWAY" do + stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + server.read_frame + + server.send_goaway(0) + client.read_frame + + expect(client).to be(:draining?) + + expect do + client.create_stream + end.to raise_exception(Protocol::HTTP2::ProtocolError, message: be =~ /Cannot create stream 3 after GOAWAY/) + + # The stream the server accepted is unaffected: + expect(client.streams.keys).to be == [1] + end + + it "still accepts the streams the remote peer initiates while draining" do + stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + server.read_frame + + server.send_goaway(0) + client.read_frame + + expect(client).to be(:draining?) + + # `last_stream_id` only covers the streams we initiate, so the server can still push on the stream it accepted. Its frames must be processed like any other: dropping them would desynchronise the HPACK context shared by the whole connection. + promised_stream = server.streams[1].send_push_promise(request_headers) + client.read_frame + + expect(client.streams.keys).to be == [1, promised_stream.id] + + promised_stream.send_headers(response_headers, Protocol::HTTP2::END_STREAM) + + expect(client.read_frame).to be_a(Protocol::HTTP2::HeadersFrame) + expect(client.streams[promised_stream.id].state).to be == :half_closed_local + end + + it "decides the state of the connection even if a stream callback raises" do + raising_stream_class = Class.new(Protocol::HTTP2::Stream) do + def closed(error) + super + + raise "Error in closed callback!" if error + end + end + + another_stream = client.create_stream do |connection, id| + raising_stream_class.create(connection, id) + end + another_stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + + # The server did not process anything, so the stream is refused and its callback raises: + server.send_goaway(0) + + expect do + client.read_frame + end.to raise_exception(RuntimeError, message: be =~ /Error in closed callback/) + + expect(client).to be(:goaway_received?) + expect(client).not.to be(:draining?) + expect(client).to be(:closed?) + end + + it "closes the connection immediately if there is nothing to drain" do + stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + server.read_frame + + # The server processes the stream before shutting down: + server.streams[1].send_headers(response_headers, Protocol::HTTP2::END_STREAM) + server.send_goaway(0) + + client.read_frame + expect(stream.state).to be == :closed + + client.read_frame + + expect(client).to be(:goaway_received?) + expect(client).not.to be(:draining?) + expect(client).to be(:closed?) end it "closes all streams with RefusedError on GOAWAY with last_stream_id=0" do From 03f7de3c1bfda2610824f013f9a62e423fcacdab Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 31 Aug 2026 09:48:01 +1200 Subject: [PATCH 2/6] Track the received GOAWAY stream ID separately. Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543 --- lib/protocol/http2/connection.rb | 30 +++++++++------- test/protocol/http2/connection.rb | 60 +++++++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/lib/protocol/http2/connection.rb b/lib/protocol/http2/connection.rb index 96e019f..31a8344 100644 --- a/lib/protocol/http2/connection.rb +++ b/lib/protocol/http2/connection.rb @@ -45,8 +45,8 @@ def initialize(framer, local_stream_id) @local_window = LocalWindow.new @remote_window = Window.new - # Whether the remote peer has sent us a GOAWAY frame: - @goaway_received = false + # The lowest Last-Stream-ID received in a GOAWAY frame, or nil if no GOAWAY frame has been received: + @goaway_stream_id = nil end # The connection stream ID (always 0 for connection-level operations). @@ -97,28 +97,31 @@ def maximum_concurrent_streams # The highest stream_id that has been successfully accepted by this connection. attr :remote_stream_id + # The lowest Last-Stream-ID received in a GOAWAY frame. + attr :goaway_stream_id + # Whether the connection is effectively or actually closed. def closed? @state == :closed || @framer.nil? end - # Whether the remote peer has sent us a GOAWAY frame. We must not initiate any new streams on this connection, but the streams at or below `remote_stream_id` may still be in progress. + # Whether the remote peer has sent us a GOAWAY frame. We must not initiate any new streams on this connection, but existing streams may still be in progress. # @returns [Boolean] True if a GOAWAY frame has been received. def goaway_received? - @goaway_received + !@goaway_stream_id.nil? end - # Whether the connection is draining the streams which were accepted by the remote peer before it sent a graceful GOAWAY frame. Such a connection is still readable and those streams will still receive their responses, but it must not be used for new requests. + # Whether the connection is draining active streams after the remote peer sent a graceful GOAWAY frame. Such a connection is still readable and those streams may still exchange frames, but it must not be used to initiate new streams. # @returns [Boolean] True if a graceful shutdown is in progress and streams are still active. def draining? - @goaway_received && !self.closed? && @streams.any? + self.goaway_received? && !self.closed? && @streams.any? end # Transition the connection into the closed state if a graceful GOAWAY was received and there is nothing left to drain. # # As with {close!}, this is a state transition only: the owner of the connection is responsible for closing the underlying framer. def close_if_drained! - if @goaway_received && @streams.empty? + if self.goaway_received? && @streams.empty? self.close! end end @@ -270,13 +273,14 @@ def send_goaway(error_code = 0, message = "") # @parameter frame [GoawayFrame] The GOAWAY frame to process. # @raises [GoawayError] If the frame indicates a connection error. def receive_goaway(frame) - # We capture the last stream that was processed. - @remote_stream_id, error_code, message = frame.unpack + # We capture the last locally-initiated stream that may have been processed by the peer. + goaway_stream_id, error_code, message = frame.unpack - @goaway_received = true + # A peer can send an initial GOAWAY with a high stream ID, followed by another GOAWAY with a lower stream ID. The effective cutoff can only decrease (RFC 9113 §6.8). + @goaway_stream_id = [@goaway_stream_id || goaway_stream_id, goaway_stream_id].min - # Streams above the last stream ID were not processed by the remote peer and are safe to retry (RFC 9113 §6.8). They are removed from the connection before being closed, both so that what remains is exactly the set of streams we are waiting on, and so that closing them cannot mutate the collection while we are traversing it. - refused_streams = @streams.select{|id, stream| id > @remote_stream_id} + # Locally-initiated streams above the last stream ID were not processed by the remote peer and are safe to retry (RFC 9113 §6.8). They are removed from the connection before being closed, both so that what remains is exactly the set of streams we are waiting on, and so that closing them cannot mutate the collection while we are traversing it. + refused_streams = @streams.select{|id, stream| local_stream_id?(id) && id > @goaway_stream_id} refused_streams.each_key{|id| @streams.delete(id)} # The state of the connection is decided before any stream is closed, so that it cannot be left undecided by a `closed` hook which raises, and cannot be influenced by one which creates a stream. @@ -477,7 +481,7 @@ def accept_push_promise_stream(stream_id, &block) # On the client side, we create requests. # @return [Stream] the created stream. def create_stream(id = next_stream_id, &block) - if @goaway_received and local_stream_id?(id) + if self.goaway_received? and local_stream_id?(id) # Receivers of a GOAWAY frame MUST NOT open additional streams on the connection (RFC 9113 §6.8). A new connection has to be established for new streams. raise ProtocolError, "Cannot create stream #{id} after GOAWAY!" end diff --git a/test/protocol/http2/connection.rb b/test/protocol/http2/connection.rb index 297339a..7686e2c 100644 --- a/test/protocol/http2/connection.rb +++ b/test/protocol/http2/connection.rb @@ -42,6 +42,10 @@ expect(connection).not.to be(:valid_remote_stream_id?, 1) end + it "does not report any stream_id as being local" do + expect(connection).not.to be(:local_stream_id?, 1) + end + it "rejects a push promise" do frame = Protocol::HTTP2::PushPromiseFrame.new @@ -374,7 +378,8 @@ def before another_stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) expect(client.read_frame).to be_a Protocol::HTTP2::GoawayFrame - expect(client.remote_stream_id).to be == 1 + expect(client.goaway_stream_id).to be == 1 + expect(client.remote_stream_id).to be == 0 # The server accepted stream 1 and is still processing it, so the connection is not closed yet: expect(client).to be(:goaway_received?) @@ -399,6 +404,21 @@ def before expect(client).to be(:closed?) end + it "keeps peer-initiated streams open when receiving GOAWAY" do + stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + server.read_frame + + # The client's GOAWAY stream ID applies to streams initiated by the server, not the client's request stream: + client.send_goaway(0) + server.read_frame + + expect(server.goaway_stream_id).to be == 0 + expect(server.remote_stream_id).to be == 1 + expect(server.streams.keys).to be == [1] + expect(server.streams[1].state).to be == :half_closed_remote + expect(server).to be(:draining?) + end + let(:stream_class) do Class.new(Protocol::HTTP2::Stream) do attr_reader :error @@ -455,6 +475,41 @@ def closed(error) expect(client.streams.keys).to be == [1] end + it "uses the lowest stream ID from successive GOAWAY frames" do + stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + + another_stream = client.create_stream do |connection, id| + stream_class.create(connection, id) + end + another_stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) + + goaway = Protocol::HTTP2::GoawayFrame.new + goaway.pack(3, 0, "") + server.write_frame(goaway) + client.read_frame + + expect(client.goaway_stream_id).to be == 3 + expect(another_stream.state).not.to be == :closed + + goaway = Protocol::HTTP2::GoawayFrame.new + goaway.pack(1, 0, "") + server.write_frame(goaway) + client.read_frame + + expect(client.goaway_stream_id).to be == 1 + expect(another_stream.state).to be == :closed + expect(another_stream.error).to be_a(Protocol::HTTP::RefusedError) + expect(client.streams.keys).to be == [1] + + goaway = Protocol::HTTP2::GoawayFrame.new + goaway.pack(3, 0, "") + server.write_frame(goaway) + client.read_frame + + # A later GOAWAY cannot raise the effective cutoff: + expect(client.goaway_stream_id).to be == 1 + end + it "drains the streams which were accepted before a graceful GOAWAY" do stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) @@ -633,7 +688,8 @@ def closed(error) client.close - expect(client.remote_stream_id).to be == 1 + expect(client.goaway_stream_id).to be == 1 + expect(client.remote_stream_id).to be == 0 expect(client).to be(:closed?) end From 3eddaf641edb20b60f01a97df9cd25207d72c389 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 31 Aug 2026 10:01:45 +1200 Subject: [PATCH 3/6] Keep generic connection close unchanged. Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543 --- lib/protocol/http2/connection.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/protocol/http2/connection.rb b/lib/protocol/http2/connection.rb index 31a8344..64a0157 100644 --- a/lib/protocol/http2/connection.rb +++ b/lib/protocol/http2/connection.rb @@ -149,10 +149,8 @@ def close(error = nil) error = EOFError.new("Connection closed with #{@streams.size} active stream(s)!") end - # The streams are detached before any of them is closed, so that a re-entrant `close` - a stream's `closed` hook can reach one, e.g. by releasing the connection back to a pool - neither sees them as active nor closes them a second time with a different error: - streams, @streams = @streams, {} - - streams.each_value{|stream| stream.close(error)} + @streams.each_value{|stream| stream.close(error)} + @streams.clear ensure if @framer From 8630f612efb4b23b7fe1a6b0c392f04248a3f255 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 31 Aug 2026 10:03:09 +1200 Subject: [PATCH 4/6] Update the GOAWAY cutoff explicitly. Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543 --- lib/protocol/http2/connection.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/protocol/http2/connection.rb b/lib/protocol/http2/connection.rb index 64a0157..6620f6d 100644 --- a/lib/protocol/http2/connection.rb +++ b/lib/protocol/http2/connection.rb @@ -275,7 +275,9 @@ def receive_goaway(frame) goaway_stream_id, error_code, message = frame.unpack # A peer can send an initial GOAWAY with a high stream ID, followed by another GOAWAY with a lower stream ID. The effective cutoff can only decrease (RFC 9113 §6.8). - @goaway_stream_id = [@goaway_stream_id || goaway_stream_id, goaway_stream_id].min + if @goaway_stream_id.nil? || goaway_stream_id < @goaway_stream_id + @goaway_stream_id = goaway_stream_id + end # Locally-initiated streams above the last stream ID were not processed by the remote peer and are safe to retry (RFC 9113 §6.8). They are removed from the connection before being closed, both so that what remains is exactly the set of streams we are waiting on, and so that closing them cannot mutate the collection while we are traversing it. refused_streams = @streams.select{|id, stream| local_stream_id?(id) && id > @goaway_stream_id} From 453b9e4c903f5e9634aee3c95ad0bed06640b080 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 31 Aug 2026 14:27:06 +1200 Subject: [PATCH 5/6] Remove stale connection close release note Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543 --- releases.md | 1 - 1 file changed, 1 deletion(-) diff --git a/releases.md b/releases.md index 47d56f7..a5bae61 100644 --- a/releases.md +++ b/releases.md @@ -4,7 +4,6 @@ - On a graceful `GOAWAY` (error code `0`), keep the connection open until the streams the remote peer accepted have completed, instead of closing it immediately and failing those requests with `EOFError`. - `Connection#create_stream` refuses to open a locally-initiated stream once a `GOAWAY` has been received, as required by RFC 9113 §6.8. - - `Connection#close` detaches the active streams before closing them, so a re-entrant close cannot report a fabricated `EOFError` in place of the real error. ## v0.26.2 From 64bd1fe4a5cf5f0980471cbc796c65b099919522 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Mon, 31 Aug 2026 14:40:21 +1200 Subject: [PATCH 6/6] Remove Connection#draining? Assisted-By: devx/618580b0-d55f-4c2e-95b6-87e648f60543 --- lib/protocol/http2/connection.rb | 6 ------ test/protocol/http2/connection.rb | 19 ++++++++----------- 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/lib/protocol/http2/connection.rb b/lib/protocol/http2/connection.rb index 6620f6d..d7b59be 100644 --- a/lib/protocol/http2/connection.rb +++ b/lib/protocol/http2/connection.rb @@ -111,12 +111,6 @@ def goaway_received? !@goaway_stream_id.nil? end - # Whether the connection is draining active streams after the remote peer sent a graceful GOAWAY frame. Such a connection is still readable and those streams may still exchange frames, but it must not be used to initiate new streams. - # @returns [Boolean] True if a graceful shutdown is in progress and streams are still active. - def draining? - self.goaway_received? && !self.closed? && @streams.any? - end - # Transition the connection into the closed state if a graceful GOAWAY was received and there is nothing left to drain. # # As with {close!}, this is a state transition only: the owner of the connection is responsible for closing the underlying framer. diff --git a/test/protocol/http2/connection.rb b/test/protocol/http2/connection.rb index 7686e2c..2e66bd1 100644 --- a/test/protocol/http2/connection.rb +++ b/test/protocol/http2/connection.rb @@ -383,7 +383,6 @@ def before # The server accepted stream 1 and is still processing it, so the connection is not closed yet: expect(client).to be(:goaway_received?) - expect(client).to be(:draining?) expect(client).not.to be(:closed?) # The server will ignore this frame as it was sent after the graceful shutdown: @@ -400,7 +399,6 @@ def before expect(stream.state).to be == :closed # There is nothing left to drain, so the connection is closed: - expect(client).not.to be(:draining?) expect(client).to be(:closed?) end @@ -416,7 +414,8 @@ def before expect(server.remote_stream_id).to be == 1 expect(server.streams.keys).to be == [1] expect(server.streams[1].state).to be == :half_closed_remote - expect(server).to be(:draining?) + expect(server).to be(:goaway_received?) + expect(server).not.to be(:closed?) end let(:stream_class) do @@ -528,7 +527,6 @@ def closed(error) client.read_frame expect(client).to be(:goaway_received?) - expect(client).to be(:draining?) expect(client).not.to be(:closed?) expect(client.streams.keys).to be == [1, 3] @@ -537,8 +535,8 @@ def closed(error) client.read_frame expect(stream.state).to be == :closed - expect(client).to be(:draining?) expect(client).not.to be(:closed?) + expect(client.streams.keys).to be == [3] server.streams[3].send_headers(response_headers, Protocol::HTTP2::END_STREAM) client.read_frame @@ -547,7 +545,6 @@ def closed(error) expect(another_stream.error).to be_nil # The last accepted stream completed, so the connection is closed: - expect(client).not.to be(:draining?) expect(client).to be(:closed?) end @@ -558,7 +555,8 @@ def closed(error) server.send_goaway(0) client.read_frame - expect(client).to be(:draining?) + expect(client).to be(:goaway_received?) + expect(client).not.to be(:closed?) expect do client.create_stream @@ -568,14 +566,15 @@ def closed(error) expect(client.streams.keys).to be == [1] end - it "still accepts the streams the remote peer initiates while draining" do + it "still accepts streams initiated by the remote peer after GOAWAY" do stream.send_headers(request_headers, Protocol::HTTP2::END_STREAM) server.read_frame server.send_goaway(0) client.read_frame - expect(client).to be(:draining?) + expect(client).to be(:goaway_received?) + expect(client).not.to be(:closed?) # `last_stream_id` only covers the streams we initiate, so the server can still push on the stream it accepted. Its frames must be processed like any other: dropping them would desynchronise the HPACK context shared by the whole connection. promised_stream = server.streams[1].send_push_promise(request_headers) @@ -611,7 +610,6 @@ def closed(error) end.to raise_exception(RuntimeError, message: be =~ /Error in closed callback/) expect(client).to be(:goaway_received?) - expect(client).not.to be(:draining?) expect(client).to be(:closed?) end @@ -629,7 +627,6 @@ def closed(error) client.read_frame expect(client).to be(:goaway_received?) - expect(client).not.to be(:draining?) expect(client).to be(:closed?) end