Avoid a double-idle in the idle handler. (#875)

Motivation:

The idle connectivity state can be achieved in two ways:
1. We have no active streams and receive a GOAWAY frame, and
2. We have no active streams and the idle timeout elapses.

The only valid state we can be in to transition to idle is ready (i.e. we have
an active channel and have received the first SETTINGS frame). We don't
currently protect against going idle twice: that is, receiving a GOAWAY
and subequently having the timeout fire. This leads to an invalid state
transition (idle to idle).

Modifications:

- Check our 'readiness' state in the idle handler before calling
  'idle()' on the connection manager
- Add a test.
- Alseo cancel the timeout when the handler is removed.

Result:

We avoid invalid state transitions when double-idling.

Author: glbrntt

Sources/GRPC/GRPCIdleHandler.swift

@@ -97,6 +97,10 @@ internal class GRPCIdleHandler: ChannelInboundHandler {
     context.fireChannelActive()
   }
 
+  func handlerRemoved(context: ChannelHandlerContext) {
+    self.scheduledIdle?.cancel()
+  }
+
   func channelInactive(context: ChannelHandlerContext) {
     self.scheduledIdle?.cancel()
     self.scheduledIdle = nil
@@ -166,18 +170,26 @@ internal class GRPCIdleHandler: ChannelInboundHandler {
   }
 
   private func idle(context: ChannelHandlerContext) {
+    // Don't idle if there are active streams.
     guard self.activeStreams == 0 else {
       return
     }
 
-    self.state = .closed
-    switch self.mode {
-    case .client(let manager):
-      manager.idle()
-    case .server:
+    switch self.state {
+    case .notReady, .ready:
+      self.state = .closed
+      switch self.mode {
+      case .client(let manager):
+        manager.idle()
+      case .server:
+        ()
+      }
+      context.close(mode: .all, promise: nil)
+
+    // We need to guard against double closure here. We may go idle as a result of receiving a
+    // GOAWAY frame or because our scheduled idle timeout fired. 
+    case .closed:
       ()
     }
-
-    context.close(mode: .all, promise: nil)
   }
 }

Tests/GRPCTests/ConnectionManagerTests.swift

@@ -687,6 +687,61 @@ extension ConnectionManagerTests {
     self.loop.run()
     XCTAssertThrowsError(try channel.wait())
   }
+
+  func testDoubleIdle() throws {
+    class CloseDroppingHandler: ChannelOutboundHandler {
+      typealias OutboundIn = Any
+      func close(context: ChannelHandlerContext, mode: CloseMode, promise: EventLoopPromise<Void>?) {
+        promise?.fail(GRPCStatus(code: .unavailable, message: "Purposefully dropping channel close"))
+      }
+    }
+
+    let channelPromise = self.loop.makePromise(of: Channel.self)
+    let manager = ConnectionManager.testingOnly(configuration: self.defaultConfiguration, logger: self.logger) {
+      return 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([
+      CloseDroppingHandler(),
+      GRPCIdleHandler(mode: .client(manager))
+    ]).wait())
+    channelPromise.succeed(channel)
+    self.loop.run()
+    XCTAssertNoThrow(try channel.connect(to: SocketAddress(unixDomainSocketPath: "/ignored")).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())
+
+    // Send a GO_AWAY; the details don't matter. This will cause the connection to go idle and the
+    // channel to close.
+    try self.waitForStateChange(from: .ready, to: .idle) {
+      let goAway = HTTP2Frame(
+        streamID: .rootStream,
+        payload: .goAway(lastStreamID: 1, errorCode: .noError, opaqueData: nil)
+      )
+      XCTAssertNoThrow(try channel.writeInbound(goAway))
+    }
+
+    // We dropped the close; now wait for the scheduled idle to fire.
+    //
+    // Previously doing this this would fail a precondition.
+    self.loop.advanceTime(by: .minutes(5))
+  }
 }
 
 internal struct Change: Hashable, CustomStringConvertible {

Tests/GRPCTests/XCTestManifests.swift

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