diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStreamsState.swift b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStreamsState.swift index acaa744b..885a31e0 100644 --- a/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStreamsState.swift +++ b/Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStreamsState.swift @@ -168,14 +168,16 @@ struct ConnectionStreamState { /// - parameters: /// - streamID: The ID of the stream to modify. /// - ignoreRecentlyReset: Whether a recently reset stream should be ignored. Should be set to `true` when receiving frames. + /// - ignoreClosed: Whether a closed stream should be ignored. Should be set to `true` when receiving window update frames. /// - modifier: A block that will be invoked to modify the stream state, if present. /// - returns: The result of the state modification, as well as any state change that occurred to the stream. @inline(__always) mutating func modifyStreamState(streamID: HTTP2StreamID, ignoreRecentlyReset: Bool, + ignoreClosed: Bool = false, _ modifier: (inout HTTP2StreamStateMachine) -> StateMachineResultWithStreamEffect) -> StateMachineResultWithStreamEffect { guard let result = self.activeStreams[streamID].autoClosingTransform(modifier) else { - return StateMachineResultWithStreamEffect(result: self.streamMissing(streamID: streamID, ignoreRecentlyReset: ignoreRecentlyReset), effect: nil) + return StateMachineResultWithStreamEffect(result: self.streamMissing(streamID: streamID, ignoreRecentlyReset: ignoreRecentlyReset, ignoreClosed: ignoreClosed), effect: nil) } if let effect = result.effect, effect.closedStream { @@ -201,7 +203,7 @@ struct ConnectionStreamState { _ modifier: (inout HTTP2StreamStateMachine) -> StateMachineResultWithStreamEffect) -> StateMachineResultWithStreamEffect { guard let result = self.activeStreams[streamID].autoClosingTransform(modifier) else { // We never ignore recently reset streams here, as this should only ever be used when *sending* frames. - return StateMachineResultWithStreamEffect(result: self.streamMissing(streamID: streamID, ignoreRecentlyReset: false), effect: nil) + return StateMachineResultWithStreamEffect(result: self.streamMissing(streamID: streamID, ignoreRecentlyReset: false, ignoreClosed: false), effect: nil) } @@ -297,8 +299,9 @@ struct ConnectionStreamState { /// - parameters: /// - streamID: The ID of the missing stream. /// - ignoreRecentlyReset: Whether a recently reset stream should be ignored. + /// - ignoreClosed: Whether a closed stream should be ignored. /// - returns: A `StateMachineResult` for this frame error. - private func streamMissing(streamID: HTTP2StreamID, ignoreRecentlyReset: Bool) -> StateMachineResult { + private func streamMissing(streamID: HTTP2StreamID, ignoreRecentlyReset: Bool, ignoreClosed: Bool) -> StateMachineResult { if ignoreRecentlyReset && self.recentlyResetStreams.contains(streamID) { return .ignoreFrame } @@ -310,8 +313,11 @@ struct ConnectionStreamState { return .connectionError(underlyingError: NIOHTTP2Errors.NoSuchStream(streamID: streamID), type: .protocolError) default: // This stream must have already been closed. - return .connectionError(underlyingError: NIOHTTP2Errors.NoSuchStream(streamID: streamID), type: .streamClosed) - + if ignoreClosed { + return .ignoreFrame + } else { + return .connectionError(underlyingError: NIOHTTP2Errors.NoSuchStream(streamID: streamID), type: .streamClosed) + } } } diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingPushPromiseState.swift b/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingPushPromiseState.swift index 1397794b..8cb5198c 100644 --- a/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingPushPromiseState.swift +++ b/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingPushPromiseState.swift @@ -55,7 +55,7 @@ extension ReceivingPushPromiseState { try self.streamState.createRemotelyPushedStream(streamID: childStreamID, remoteInitialWindowSize: self.remoteInitialWindowSize) - let result = self.streamState.modifyStreamState(streamID: originalStreamID, ignoreRecentlyReset: true) { + let result = self.streamState.modifyStreamState(streamID: originalStreamID, ignoreRecentlyReset: true) { $0.receivePushPromise(headers: headers, validateHeaderBlock: validateHeaderBlock) } return StateMachineResultWithEffect(result, connectionState: self) diff --git a/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingWindowUpdateState.swift b/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingWindowUpdateState.swift index 5b58272e..721e5a66 100644 --- a/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingWindowUpdateState.swift +++ b/Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingWindowUpdateState.swift @@ -41,7 +41,7 @@ extension ReceivingWindowUpdateState { } } else { // This is an update for a specific stream: it's responsible for policing any errors. - let result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: true) { + let result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: true, ignoreClosed: true) { $0.receiveWindowUpdate(windowIncrement: increment) } return StateMachineResultWithEffect(result, connectionState: self) diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift index 8ebf8d80..3edb98c2 100644 --- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift +++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift @@ -68,6 +68,7 @@ extension ConnectionStateMachineTests { ("testDataFramesWithoutEndStream", testDataFramesWithoutEndStream), ("testSendingCompleteRequestBeforeResponse", testSendingCompleteRequestBeforeResponse), ("testWindowUpdateValidity", testWindowUpdateValidity), + ("testWindowUpdateOnClosedStream", testWindowUpdateOnClosedStream), ("testWindowIncrementsOfSizeZeroArentOk", testWindowIncrementsOfSizeZeroArentOk), ("testCannotSendDataFrames", testCannotSendDataFrames), ("testChangingInitialWindowSizeLotsOfStreams", testChangingInitialWindowSizeLotsOfStreams), diff --git a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift index 7128392f..87a77113 100644 --- a/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift +++ b/Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift @@ -427,12 +427,12 @@ class ConnectionStateMachineTests: XCTestCase { self.setupServerGoaway(streamsToOpen: [streamOne, streamThree, streamFive, streamSeven], lastStreamID: streamThree, expectedToClose: [streamFive, streamSeven]) - // Server attempts to send on a closed stream fails, and clients reject that attempt as well. + // Server attempts to send on a closed stream fails, and clients ignore that attempt. // Client attempts to send on a closed stream fails, but the server ignores such frames. var temporaryServer = self.server! var temporaryClient = self.client! assertConnectionError(type: .streamClosed, temporaryServer.sendWindowUpdate(streamID: streamFive, windowIncrement: 15)) - assertConnectionError(type: .streamClosed, temporaryClient.receiveWindowUpdate(streamID: streamFive, windowIncrement: 15)) + assertIgnored(temporaryClient.receiveWindowUpdate(streamID: streamFive, windowIncrement: 15)) temporaryServer = self.server! temporaryClient = self.client! @@ -551,7 +551,7 @@ class ConnectionStateMachineTests: XCTestCase { self.setupClientGoaway(clientStreamID: streamOne, streamsToOpen: [streamTwo, streamFour, streamSix], lastStreamID: streamTwo, expectedToClose: [streamFour, streamSix]) // Server attempts to send on a closed stream fails, but clients ignore that attempt. - // Client attempts to send on a closed stream fails, and the server rejects such frames. + // Client attempts to send on a closed stream fails, but the server ignores that attempt. var temporaryServer = self.server! var temporaryClient = self.client! assertConnectionError(type: .streamClosed, temporaryServer.sendWindowUpdate(streamID: streamFour, windowIncrement: 15)) @@ -560,7 +560,7 @@ class ConnectionStateMachineTests: XCTestCase { temporaryServer = self.server! temporaryClient = self.client! assertConnectionError(type: .streamClosed, temporaryClient.sendWindowUpdate(streamID: streamFour, windowIncrement: 15)) - assertConnectionError(type: .streamClosed, temporaryServer.receiveWindowUpdate(streamID: streamFour, windowIncrement: 15)) + assertIgnored(temporaryServer.receiveWindowUpdate(streamID: streamFour, windowIncrement: 15)) } func testRstStreamOnClosedStreamAfterClientGoaway() { @@ -1315,6 +1315,36 @@ class ConnectionStateMachineTests: XCTestCase { assertStreamError(type: .protocolError, tempClient.receiveWindowUpdate(streamID: streamTwo, windowIncrement: 15)) } + func testWindowUpdateOnClosedStream() { + let streamOne = HTTP2StreamID(1) + + self.exchangePreamble() + + // Client sends a request. + assertSucceeds(self.client.sendHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: false)) + assertSucceeds(self.server.receiveHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: false)) + + // Server sends a response. + assertSucceeds(self.server.sendHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: false)) + assertSucceeds(self.client.receiveHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.responseHeaders, isEndStreamSet: false)) + + // Client sends end stream, server receives end stream. + assertSucceeds(self.client.sendData(streamID: streamOne, contentLength: 0, flowControlledBytes: 0, isEndStreamSet: true)) + assertSucceeds(self.server.receiveData(streamID: streamOne, contentLength: 0, flowControlledBytes: 0, isEndStreamSet: true)) + + // Client is now half closed (local), server is half closed (remote) + + // Server sends end stream and is now closed. + assertSucceeds(self.server.sendData(streamID: streamOne, contentLength: 0, flowControlledBytes: 0, isEndStreamSet: true)) + + // Client is half closed and sends window update, the server MUST ignore this. + assertSucceeds(self.client.sendWindowUpdate(streamID: streamOne, windowIncrement: 10)) + assertIgnored(self.server.receiveWindowUpdate(streamID: streamOne, windowIncrement: 10)) + + // Client receives end stream and closes. + assertSucceeds(self.client.receiveData(streamID: streamOne, contentLength: 0, flowControlledBytes: 0, isEndStreamSet: true)) + } + func testWindowIncrementsOfSizeZeroArentOk() { let streamOne = HTTP2StreamID(1)