Soften errors when seeing inactive before active (#376)

Motivation:

When we started checking active/inactive ordering, we added code that
rejected some channel pipeline state transitions. That code was too
strict: it is absolutely possible to see channelInactive before
channelActive in well-functioning programs.

We'll still call this an error, but we don't need to crash in debug mode
when we see it.

Modifications:

- Made channelInactive before channelActive tolerated.

Result:

More reliable code

Author: Lukasa

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -222,16 +222,21 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
             // call down the pipeline when we added in handlerAdded. That's ok!
             // We can safely ignore this.
             return
-        case .activating, .inactiveWhileActivating, .inactive:
+        case .inactive:
+            // This means we received channelInactive already. This can happen, unfortunately, usually when
+            // calling close() from within the completion of a channel connect promise. We tolerate this,
+            // but fire an error to make sure it's fatal.
+            context.fireChannelActive()
+            context.fireErrorCaught(NIOHTTP2Errors.ActivationError(state: .inactive, activating: true))
+            return
+        case .activating, .inactiveWhileActivating:
             // All of these states are channel pipeline invariant violations, but conceptually possible.
             //
             // - .activating implies we got another channelActive while we were handling one. That would be
             //     a violation of pipeline invariants.
             // - .inactiveWhileActivating implies a sequence of channelActive, channelInactive, channelActive
             //     synchronously. This is not just unlikely, but also misuse of the handler or violation of
             //     channel pipeline invariants.
-            // - .inactive implies we received channelInactive and then got another active. This is almost certainly
-            //     misuse of the handler.
             //
             // We'll throw an error and then close. In debug builds, we crash.
             self.impossibleActivationStateTransition(
@@ -257,7 +262,7 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
         case .idle, .activated, .inactive:
             // These three states should be impossible.
             //
-            // - .idle somehow implies we didn't execute the code above.
+            // - .idle and .inactive somehow implies we didn't execute the code above.
             // - .activated implies that the above code didn't prevent us re-entrantly getting to this point.
             // - .inactive implies that somehow we hit channelInactive but didn't enter .inactiveWhileActivating.
             self.impossibleActivationStateTransition(
@@ -277,12 +282,12 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
             // Huh, we got channelInactive during activation. We need to maintain
             // ordering, so we'll temporarily delay this.
             self.activationState = .inactiveWhileActivating
-        case .idle, .inactiveWhileActivating, .inactive:
-            // This is weird.
-            //
-            // .idle implies that somehow we got channelInactive before channelActive, which is probably an error.
-            // .inactiveWhileActivating and .inactive make this a second channelInactive call, which is also probably
-            //    an error.
+        case .idle:
+            // This implies getting channelInactive before channelActive. This is uncommon, but not impossible.
+            // We'll tolerate it here.
+            self.activationState = .inactive
+        case .inactiveWhileActivating, .inactive:
+            // This is weird. This can only happen if we see channelInactive twice, which is probably an error.
             self.impossibleActivationStateTransition(state: self.activationState, activating: false, context: context)
         }
 

Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift

@@ -36,6 +36,7 @@ extension SimpleClientServerTests {
                 ("testHandlingChannelInactiveDuringActive", testHandlingChannelInactiveDuringActive),
                 ("testWritingFromChannelActiveIsntReordered", testWritingFromChannelActiveIsntReordered),
                 ("testChannelActiveAfterAddingToActivePipelineDoesntError", testChannelActiveAfterAddingToActivePipelineDoesntError),
+                ("testChannelInactiveThenChannelActiveErrorsButDoesntTrap", testChannelInactiveThenChannelActiveErrorsButDoesntTrap),
                 ("testImpossibleStateTransitionsThrowErrors", testImpossibleStateTransitionsThrowErrors),
                 ("testDynamicHeaderFieldsArentEmittedWithZeroTableSize", testDynamicHeaderFieldsArentEmittedWithZeroTableSize),
                 ("testDynamicHeaderFieldsArentToleratedWithZeroTableSize", testDynamicHeaderFieldsArentToleratedWithZeroTableSize),

Tests/NIOHTTP2Tests/SimpleClientServerTests.swift

@@ -428,6 +428,29 @@ class SimpleClientServerTests: XCTestCase {
         XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self))
     }
 
+    func testChannelInactiveThenChannelActiveErrorsButDoesntTrap() throws {
+        let channel = EmbeddedChannel()
+        let recorder = ErrorRecorder()
+        try channel.pipeline.syncOperations.addHandlers(
+            [
+                NIOHTTP2Handler(mode: .client),
+                recorder
+            ]
+        )
+
+        XCTAssertEqual(recorder.errors.count, 0)
+
+        // Send channelInactive followed by channelActive. This can happen if a user calls close
+        // from within a connect promise.
+        channel.pipeline.fireChannelInactive()
+        XCTAssertEqual(recorder.errors.count, 0)
+
+        channel.pipeline.fireChannelActive()
+
+        XCTAssertEqual(recorder.errors.count, 1)
+        XCTAssertTrue(recorder.errors.allSatisfy { $0 is NIOHTTP2Errors.ActivationError })
+    }
+
     func testImpossibleStateTransitionsThrowErrors() throws {
         func setUpChannel() throws -> (EmbeddedChannel, ErrorRecorder) {
             let channel = EmbeddedChannel()
@@ -443,22 +466,14 @@ class SimpleClientServerTests: XCTestCase {
             return (channel, recorder)
         }
 
-        // First impossible state transition: channelInactive during idle.
-        // Doesn't transition state, just errors. Becuase we close during this, we hit
-        // the error twice!
+        // First impossible state transition: channelActive on channelActive.
         var (channel, recorder) = try setUpChannel()
-        channel.pipeline.fireChannelInactive()
-        XCTAssertEqual(recorder.errors.count, 2)
-        XCTAssertTrue(recorder.errors.allSatisfy { $0 is NIOHTTP2Errors.ActivationError })
-
-        // Second impossible state transition: channelActive on channelActive.
-        (channel, recorder) = try setUpChannel()
         try channel.pipeline.syncOperations.addHandler(ActionOnFlushHandler { $0.pipeline.fireChannelActive() }, position: .first)
         channel.pipeline.fireChannelActive()
         XCTAssertEqual(recorder.errors.count, 1)
         XCTAssertTrue(recorder.errors.allSatisfy { $0 is NIOHTTP2Errors.ActivationError })
 
-        // Third impossible state transition. Synchronous active/inactive/active. The error causes a close,
+        // Second impossible state transition. Synchronous active/inactive/active. The error causes a close,
         // so we error twice!
         (channel, recorder) = try setUpChannel()
         try channel.pipeline.syncOperations.addHandler(
@@ -472,17 +487,18 @@ class SimpleClientServerTests: XCTestCase {
         XCTAssertEqual(recorder.errors.count, 2)
         XCTAssertTrue(recorder.errors.allSatisfy { $0 is NIOHTTP2Errors.ActivationError })
 
-        // Fourth impossible state transition: active/inactive/active asynchronously. The error causes a close,
-        // so we error twice!
+        // Third impossible state transition: active/inactive/active asynchronously. This error doesn't cause a
+        // close because we don't distinguish it from the case tested in
+        // testChannelInactiveThenChannelActiveErrorsButDoesntTrap.
         (channel, recorder) = try setUpChannel()
         channel.pipeline.fireChannelActive()
         channel.pipeline.fireChannelInactive()
         XCTAssertEqual(recorder.errors.count, 0)
         channel.pipeline.fireChannelActive()
-        XCTAssertEqual(recorder.errors.count, 2)
+        XCTAssertEqual(recorder.errors.count, 1)
         XCTAssertTrue(recorder.errors.allSatisfy { $0 is NIOHTTP2Errors.ActivationError })
 
-        // Fifth impossible state transition: active/inactive/inactive synchronously. The error causes a close,
+        // Fourth impossible state transition: active/inactive/inactive synchronously. The error causes a close,
         // so we error twice!
         (channel, recorder) = try setUpChannel()
         try channel.pipeline.syncOperations.addHandler(
@@ -496,7 +512,7 @@ class SimpleClientServerTests: XCTestCase {
         XCTAssertEqual(recorder.errors.count, 2)
         XCTAssertTrue(recorder.errors.allSatisfy { $0 is NIOHTTP2Errors.ActivationError })
 
-        // Sixth impossible state transition: active/inactive/inactive asynchronously. The error causes a close,
+        // Fifth impossible state transition: active/inactive/inactive asynchronously. The error causes a close,
         // so we error twice!
         (channel, recorder) = try setUpChannel()
         channel.pipeline.fireChannelActive()
@@ -506,7 +522,7 @@ class SimpleClientServerTests: XCTestCase {
         XCTAssertEqual(recorder.errors.count, 2)
         XCTAssertTrue(recorder.errors.allSatisfy { $0 is NIOHTTP2Errors.ActivationError })
 
-        // Seventh impossible state transition: adding the handler twice. The error causes a close, so we
+        // Sixth impossible state transition: adding the handler twice. The error causes a close, so we
         // error twice!
         (channel, recorder) = try setUpChannel()
         try channel.connect(to: SocketAddress(unixDomainSocketPath: "/tmp/ignored"), promise: nil)