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.

Sign up for GitHub

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

Fix bug where client ignores HEADERS frames on open stream when locally quiescing #445

Merged
merged 6 commits into from
Jul 18, 2024
Merged

Fix bug where client ignores HEADERS frames on open stream when locally quiescing #445

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e7bb7d4
Fix bug where client incorrectly ignores HEADERS frames on open strea…
aryan-25 Jul 4, 2024
9305b9a
Refactor
aryan-25 Jul 5, 2024
ae7c4c5
Refactor
aryan-25 Jul 5, 2024
6340096
Revert new line changes
aryan-25 Jul 5, 2024
71e6ecd
Revert switch case change
aryan-25 Jul 5, 2024
d3bd74b
Merge branch 'main' into main
glbrntt Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStateMachine.swift
View file
Open in desktop
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
7 changes: 5 additions & 2 deletions Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingHeadersState.swift
View file
Open in desktop
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the protocol constraint here at all? We already have the role.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ConnectionStateWithRole has a member peerRole which is being used as part of the fix:

image

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
View file
Open in desktop
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