Move idle handler state to closed on channelInactive (#953)

Motivation:

Related to #949.

It's possible for the client's connection to be dropped, and for the
connection state to become idle, prior to the scheduled close from the
keepalive handler firing. As the idle handler never updates its state to
'closed' in 'channelInactive', when the scheduled close event is picked
up in the idle handler it effectively ask the connection manager to
double-idle, hitting a precondition failure.

Modifications:

- Move the 'GRPCIdleHandler' to '.closed' in 'channelInactive'
- Cancel scheduled pings and timeouts on 'channelInactive' in the
  'GRPCKeepaliveHandlers'

Result:

- We avoid another path to a precondition failure

Sources/GRPC/GRPCIdleHandler.swift

@@ -117,6 +117,8 @@ internal class GRPCIdleHandler: ChannelInboundHandler {
 
   func handlerRemoved(context: ChannelHandlerContext) {
     self.scheduledIdle?.cancel()
+    self.scheduledIdle = nil
+    self.state = .closed
   }
 
   func channelInactive(context: ChannelHandlerContext) {
@@ -126,12 +128,13 @@ internal class GRPCIdleHandler: ChannelInboundHandler {
     switch (self.mode, self.state) {
     case let (.client(manager), .notReady),
          let (.client(manager), .ready):
+      self.state = .closed
       manager.channelInactive()
 
     case (.server, .notReady),
          (.server, .ready),
          (_, .closed):
-      ()
+      self.state = .closed
     }
 
     context.fireChannelInactive()

Sources/GRPC/GRPCKeepaliveHandlers.swift

@@ -101,6 +101,12 @@ extension _ChannelKeepaliveHandler {
     context.fireChannelRead(data)
   }
 
+  func channelInactive(context: ChannelHandlerContext) {
+    self.cancelScheduledPing()
+    self.cancelScheduledTimeout()
+    context.fireChannelInactive()
+  }
+
   func handlerRemoved(context: ChannelHandlerContext) {
     self.cancelScheduledPing()
     self.cancelScheduledTimeout()

Tests/GRPCTests/ConnectionManagerTests.swift

@@ -824,6 +824,55 @@ extension ConnectionManagerTests {
     // Previously doing this this would fail a precondition.
     self.loop.advanceTime(by: .minutes(5))
   }
+
+  func testForceIdleAfterInactive() throws {
+    let channelPromise = self.loop.makePromise(of: Channel.self)
+    let manager = ConnectionManager.testingOnly(
+      configuration: self.defaultConfiguration,
+      logger: self.logger
+    ) {
+      channelPromise.futureResult
+    }
+
+    // Start the connection.
+    let readyChannel: EventLoopFuture<Channel> = self.waitForStateChange(
+      from: .idle,
+      to: .connecting
+    ) {
+      let readyChannel = manager.getChannel()
+      self.loop.run()
+      return readyChannel
+    }
+
+    // Setup the real channel and activate it.
+    let channel = EmbeddedChannel(loop: self.loop)
+    XCTAssertNoThrow(try channel.pipeline.addHandlers([
+      GRPCIdleHandler(mode: .client(manager), logger: manager.logger),
+    ]).wait())
+    channelPromise.succeed(channel)
+    self.loop.run()
+
+    let connect = channel.connect(to: try SocketAddress(unixDomainSocketPath: "/ignored"))
+    XCTAssertNoThrow(try connect.wait())
+
+    // Write a SETTINGS frame on the root stream.
+    try self.waitForStateChange(from: .connecting, to: .ready) {
+      let frame = HTTP2Frame(streamID: .rootStream, payload: .settings(.settings([])))
+      XCTAssertNoThrow(try channel.writeInbound(frame))
+    }
+
+    // The channel should now be ready.
+    XCTAssertNoThrow(try readyChannel.wait())
+
+    // Now drop the connection.
+    self.waitForStateChange(from: .ready, to: .shutdown) {
+      channel.pipeline.fireChannelInactive()
+    }
+
+    // Fire a connection idled event, i.e. keepalive timeout has fired. This should be a no-op.
+    // Previously this would hit a precondition failure.
+    channel.pipeline.fireUserInboundEventTriggered(ConnectionIdledEvent())
+  }
 }
 
 internal struct Change: Hashable, CustomStringConvertible {

Tests/GRPCTests/XCTestManifests.swift

@@ -155,6 +155,7 @@ extension ConnectionManagerTests {
         ("testDoomedOptimisticChannelFromConnecting", testDoomedOptimisticChannelFromConnecting),
         ("testDoomedOptimisticChannelFromIdle", testDoomedOptimisticChannelFromIdle),
         ("testDoubleIdle", testDoubleIdle),
+        ("testForceIdleAfterInactive", testForceIdleAfterInactive),
         ("testGoAwayWhenReady", testGoAwayWhenReady),
         ("testIdleShutdown", testIdleShutdown),
         ("testIdleTimeoutWhenThereAreActiveStreams", testIdleTimeoutWhenThereAreActiveStreams),