Ignore WINDOW_UPDATE on closed streams. (#109)

Motivation:

RFC 7540 Section 6.9: "WINDOW_UPDATE can be sent by a peer that has sent
a frame bearing the END_STREAM flag.  This means that a receiver could
receive a WINDOW_UPDATE frame on a "half-closed (remote)" or "closed"
stream. A receiver MUST NOT treat this as an error (see Section 5.1)."

Previously WINDOW_UPDATE frames received in this state would be treated
as an error.

Modifications:

- Allow frames to be ignored if they are for a stream which has been
  closed.
- Add a test to verify WINDOW_UPDATE on a closed stream.
- Update tests which relied on a stream error in this case.

Result:

More compliant code.

Author: glbrntt

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)
+            }
         }
     }
 

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)

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)

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),

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)