Skip to content

Commit

Permalink
Fix bug where client ignores HEADERS frames on open stream when local…
Browse files Browse the repository at this point in the history
…ly quiescing (#445)

Motivation:

Currently, HEADERS frames are ignored by the client on a client-initiated stream after the client sends a GOAWAY.

As per [Section 6.8 (GOAWAY) in RFC 9113](https://httpwg.org/specs/rfc9113#GOAWAY):
_"Once the GOAWAY is sent, the sender will ignore frames sent on streams **initiated by the receiver** if the stream has an identifier higher than the included last stream identifier."_

In this case, the client (sender) should **not** ignore the HEADERS frame as the stream is initiated by the _client_, not the receiver.

Modifications:

Fixed the condition in the `receiveHeaders` function (given `LocallyQuiescingState`) in `ReceivingHeadersState.swift` to work for _both_ the client and server roles -- before this change, the condition is only correct when `self.role == ConnectionRole.server`

Result:

HEADERS frames are not ignored by the client on a client-initiated stream when locally quiescing.
  • Loading branch information
aryan-25 authored Jul 18, 2024
1 parent ab04d05 commit 35b0b8f
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1755,7 +1755,7 @@ extension HTTP2StreamID {


/// A simple protocol that provides helpers that apply to all connection states that keep track of a role.
private protocol ConnectionStateWithRole {
protocol ConnectionStateWithRole {
var role: HTTP2ConnectionStateMachine.ConnectionRole { get }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import NIOHPACK
/// can validly accept headers.
///
/// This protocol should only be conformed to by states for the HTTP/2 connection state machine.
protocol ReceivingHeadersState: HasFlowControlWindows, HasLocalExtendedConnectSettings, HasRemoteExtendedConnectSettings {
protocol ReceivingHeadersState: HasFlowControlWindows, HasLocalExtendedConnectSettings, HasRemoteExtendedConnectSettings, ConnectionStateWithRole {
var role: HTTP2ConnectionStateMachine.ConnectionRole { get }

var headerBlockValidation: HTTP2ConnectionStateMachine.ValidationState { get }
Expand Down Expand Up @@ -74,7 +74,10 @@ extension ReceivingHeadersState where Self: LocallyQuiescingState {
let localSupportsExtendedConnect = self.localSupportsExtendedConnect
let remoteSupportsExtendedConnect = self.remoteSupportsExtendedConnect

if streamID.mayBeInitiatedBy(.client) && streamID > self.lastRemoteStreamID {
// We are in `LocallyQuiescingState`. The sender of the GOAWAY is `self.role`, so the remote peer is the inverse of `self.role`.
let remotePeer = self.peerRole

if streamID.mayBeInitiatedBy(remotePeer) && streamID > self.lastRemoteStreamID {
return StateMachineResultWithEffect(result: .ignoreFrame, effect: nil)
}

Expand Down
23 changes: 23 additions & 0 deletions Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,29 @@ class ConnectionStateMachineTests: XCTestCase {
XCTAssertTrue(self.client.fullyQuiesced)
}

func testHeadersOnOpenStreamLocallyQuiescing() {
let streamOne = HTTP2StreamID(1)

self.exchangePreamble()

// Client sends headers
assertSucceeds(client.sendHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: false))
assertSucceeds(server.receiveHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: false))

// Server responds with headers
assertSucceeds(server.sendHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: false))
assertSucceeds(client.receiveHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: false))

// Client sends a GOAWAY with lastStreamID = 0
assertGoawaySucceeds(client.sendGoaway(lastStreamID: 0), droppingStreams: nil)
assertGoawaySucceeds(server.receiveGoaway(lastStreamID: 0), droppingStreams: nil)

// Server sends a header frame with end stream = true
let headerFrame = HPACKHeaders([("content-length", "0")])
assertSucceeds(server.sendHeaders(streamID: streamOne, headers: headerFrame, isEndStreamSet: true))
assertSucceeds(client.receiveHeaders(streamID: streamOne, headers: headerFrame, isEndStreamSet: true))
}

func testImplicitConnectionCompletion() {
// Connections can become totally idle by way of the server quiescing the client, and then having no outstanding streams.
// This test validates that we spot it and consider the connection closed at this stage.
Expand Down

0 comments on commit 35b0b8f

Please sign in to comment.