Skip to content

Commit

Permalink
Fix last peer initiated stream ID when quiescing (#1700)
Browse files Browse the repository at this point in the history
Motivation:

The idle handler records the ID of the last stream created by the remote
peer and uses it when sending GOAWAY frames. In most paths this was
correctly updated according to the role the handler played in the
connection and the stream ID. However, in the quiescing state it was
unconditionally updated. This can lead to cases where the client
attempts to send a GOAWAY with a client initiated stream ID: this is
invalid and NIO HTTP/2 treats it as a connection level error, closing
all open streams.

Modification:

- Conditionally update the last peer initiated stream ID when quiescing

Result:

Fewer connection errors
  • Loading branch information
glbrntt authored Nov 1, 2023
1 parent 765ff32 commit aeff23a
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 1 deletion.
10 changes: 9 additions & 1 deletion Sources/GRPC/GRPCIdleHandlerStateMachine.swift
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,15 @@ struct GRPCIdleHandlerStateMachine {
operations.cancelIdleTask(state.idleTask)

case var .quiescing(state):
state.lastPeerInitiatedStreamID = streamID
switch state.role {
case .client where streamID.isServerInitiated:
state.lastPeerInitiatedStreamID = streamID
case .server where streamID.isClientInitiated:
state.lastPeerInitiatedStreamID = streamID
default:
()
}

state.openStreams += 1
self.state = .quiescing(state)

Expand Down
28 changes: 28 additions & 0 deletions Tests/GRPCTests/GRPCIdleHandlerStateMachineTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,34 @@ class GRPCIdleHandlerStateMachineTests: GRPCTestCase {
_ = stateMachine.channelInactive()
stateMachine.ratchetDownGoAwayStreamID().assertDoNothing()
}

func testStreamIDWhenQuiescing() {
var stateMachine = self.makeClientStateMachine()
let op1 = stateMachine.receiveSettings([])
op1.assertConnectionManager(.ready)

// Open a stream so we enter quiescing when receiving the GOAWAY.
let op2 = stateMachine.streamCreated(withID: 1)
op2.assertDoNothing()

let op3 = stateMachine.receiveGoAway()
op3.assertConnectionManager(.quiescing)

// Create a new stream. This can happen if the GOAWAY races with opening the stream; HTTP2 will
// open and then close the stream with an error.
let op4 = stateMachine.streamCreated(withID: 3)
op4.assertDoNothing()

// Close the newly opened stream.
let op5 = stateMachine.streamClosed(withID: 3)
op5.assertDoNothing()

// Close the original stream.
let op6 = stateMachine.streamClosed(withID: 1)
// Now we can send a GOAWAY with stream ID zero (we're the client and the server didn't open
// any streams).
XCTAssertEqual(op6.sendGoAwayWithLastPeerInitiatedStreamID, 0)
}
}

extension GRPCIdleHandlerStateMachine.Operations {
Expand Down

0 comments on commit aeff23a

Please sign in to comment.