tolerate multiple RST_STREAMs for same stream (#171)

Motivation:

The HTTP/2 RFC does not explicitly forbid sending more than one
RST_STREAM for the same stream. Given that we have seen this behaviour
in the wild we are going to ignore any additional RST_STREAMs.

A further indicative that this is the right call is that the RFC
explicitly mentions that sending RST_STREAMs in the idle state is not
acceptable.

Modifications:

Ignore RST_STREAM frames for streams in the closed state.

Result:

More compatibility.

Author: weissi

Sources/NIOHTTP2/ConnectionStateMachine/ConnectionStreamsState.swift

@@ -168,7 +168,7 @@ 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.
+    ///     - ignoreClosed: Whether a closed stream should be ignored. Should be set to `true` when receiving window update or reset stream 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)

Sources/NIOHTTP2/ConnectionStateMachine/FrameReceivingStates/ReceivingRstStreamState.swift

@@ -23,7 +23,9 @@ protocol ReceivingRstStreamState: HasFlowControlWindows {
 extension ReceivingRstStreamState {
     /// Called to receive a RST_STREAM frame.
     mutating func receiveRstStream(streamID: HTTP2StreamID, reason: HTTP2ErrorCode) -> StateMachineResultWithEffect {
-        let result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: true) {
+        // RFC 7540 § 6.4 <https://httpwg.org/specs/rfc7540.html#RST_STREAM> does not explicitly forbid a peer sending
+        // multiple RST_STREAMs for the same stream which means we should ignore subsequent RST_STREAMs.
+        let result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: true, ignoreClosed: true) {
             $0.receiveRstStream(reason: reason)
         }
         return StateMachineResultWithEffect(result, connectionState: self)

Tests/NIOHTTP2Tests/ConnectionStateMachineTests+XCTest.swift

@@ -139,6 +139,7 @@ extension ConnectionStateMachineTests {
                 ("testNoPolicingMissingContentLengthForResponsesWhenValidationDisabled", testNoPolicingMissingContentLengthForResponsesWhenValidationDisabled),
                 ("testNoPolicingInvalidContentLengthForRequestsWithEndStreamWhenValidationDisabled", testNoPolicingInvalidContentLengthForRequestsWithEndStreamWhenValidationDisabled),
                 ("testNoPolicingInvalidContentLengthForResponsesWithEndStreamWhenValidationDisabled", testNoPolicingInvalidContentLengthForResponsesWithEndStreamWhenValidationDisabled),
+                ("testWeTolerateOneStreamBeingResetTwice", testWeTolerateOneStreamBeingResetTwice),
            ]
    }
 }

Tests/NIOHTTP2Tests/ConnectionStateMachineTests.swift

@@ -455,7 +455,8 @@ class ConnectionStateMachineTests: XCTestCase {
         var temporaryServer = self.server!
         var temporaryClient = self.client!
         assertConnectionError(type: .streamClosed, temporaryServer.sendRstStream(streamID: streamFive, reason: .noError))
-        assertConnectionError(type: .streamClosed, temporaryClient.receiveRstStream(streamID: streamFive, reason: .noError))
+        // We ignore RST_STREAM in the closed state because the RFC does not explicitly forbid that.
+        assertIgnored(temporaryClient.receiveRstStream(streamID: streamFive, reason: .noError))
 
         temporaryServer = self.server!
         temporaryClient = self.client!
@@ -583,7 +584,8 @@ class ConnectionStateMachineTests: XCTestCase {
         temporaryServer = self.server!
         temporaryClient = self.client!
         assertConnectionError(type: .streamClosed, temporaryClient.sendRstStream(streamID: streamFour, reason: .noError))
-        assertConnectionError(type: .streamClosed, temporaryServer.receiveRstStream(streamID: streamFour, reason: .noError))
+        // We ignore RST_STREAM in the closed state because the RFC does not explicitly forbid that.
+        assertIgnored(temporaryServer.receiveRstStream(streamID: streamFour, reason: .noError))
     }
 
     func testServerMayNotInitiateNewStreamAfterClientGoaway() {
@@ -2878,6 +2880,28 @@ class ConnectionStateMachineTests: XCTestCase {
         assertSucceeds(self.server.sendHeaders(streamID: streamOne, headers: responseHeaders, isEndStreamSet: true))
         assertSucceeds(self.client.receiveHeaders(streamID: streamOne, headers: responseHeaders, isEndStreamSet: true))
     }
+
+    func testWeTolerateOneStreamBeingResetTwice() {
+        self.exchangePreamble()
+
+        let streamOne = HTTP2StreamID(1)
+        // Set up the connection
+        assertSucceeds(self.client.sendHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: true))
+        assertSucceeds(self.server.receiveHeaders(streamID: streamOne, headers: ConnectionStateMachineTests.requestHeaders, isEndStreamSet: true))
+
+        // The server succeeds
+        let responseHeaders = HPACKHeaders([(":status", "200"), ("content-length", "25")])
+        assertSucceeds(self.server.sendHeaders(streamID: streamOne, headers: responseHeaders, isEndStreamSet: false))
+        assertSucceeds(self.client.receiveHeaders(streamID: streamOne, headers: responseHeaders, isEndStreamSet: false))
+
+        // Let's test that we cannot emit two RST_STREAMs for the same stream.
+        assertSucceeds(self.client.sendRstStream(streamID: streamOne, reason: .cancel))
+        assertConnectionError(type: .streamClosed, self.client.sendRstStream(streamID: streamOne, reason: .streamClosed))
+
+        // But let's also verify that we're happily ignoring this should the other peer do so.
+        assertSucceeds(self.server.receiveRstStream(streamID: streamOne, reason: .cancel))
+        assertIgnored(self.server.receiveRstStream(streamID: streamOne, reason: .streamClosed))
+    }
 }