From 4f8cbe0132a4550560fdeff2e190903e09c43b26 Mon Sep 17 00:00:00 2001
From: George Barnett <gbarnett@apple.com>
Date: Fri, 8 Dec 2023 16:11:52 +0000
Subject: [PATCH] Avoid invalid state when a connect failed (#1739)

Motivation:

The connection manager gets notified when a connect attempt fails and
expected to be in the connecting state. Any other state is invalid and
will lead to an assertion failure.

The manager is notified of connection failures via the channel promise
and via the handler (i.e. on `errorCaught(context:error:)`). However,
connection failures lead to a state change and will result in crashes in
debug builds if a notification is received via both paths.

Modifications:

- Ignore errors from the channel while in the connecting state and only
  listen errors from the channel promise callback.
- Add test

Result:

Fewer crashes.
---
 Sources/GRPC/ConnectionManager.swift         |  4 +-
 Tests/GRPCTests/ConnectionManagerTests.swift | 48 +++++++++++++++++++-
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/Sources/GRPC/ConnectionManager.swift b/Sources/GRPC/ConnectionManager.swift
index a0081fe7a..157be809f 100644
--- a/Sources/GRPC/ConnectionManager.swift
+++ b/Sources/GRPC/ConnectionManager.swift
@@ -657,7 +657,9 @@ internal final class ConnectionManager: @unchecked Sendable {
       )
 
     case .connecting:
-      self.connectionFailed(withError: error)
+      // Ignore the error, the channel promise will notify the manager of any error which occurs
+      // while connecting.
+      ()
 
     case var .active(state):
       state.error = error
diff --git a/Tests/GRPCTests/ConnectionManagerTests.swift b/Tests/GRPCTests/ConnectionManagerTests.swift
index 270e69835..9fb0722af 100644
--- a/Tests/GRPCTests/ConnectionManagerTests.swift
+++ b/Tests/GRPCTests/ConnectionManagerTests.swift
@@ -1272,12 +1272,58 @@ extension ConnectionManagerTests {
     }
 
     self.waitForStateChange(from: .connecting, to: .shutdown) {
-      manager.channelError(EventLoopError.shutdown)
+      channelPromise.fail(EventLoopError.shutdown)
     }
 
     XCTAssertThrowsError(try multiplexer.wait())
   }
 
+  func testChannelErrorAndConnectFailWhenConnecting() throws {
+    // This test checks a path through the connection manager which previously led to an invalid
+    // state (a connect failure in a state other than connecting). To trigger these we need to
+    // fire an error down the pipeline containing the idle handler and fail the connect promise.
+    let escapedChannelPromise = self.loop.makePromise(of: Channel.self)
+    let channelPromise = self.loop.makePromise(of: Channel.self)
+
+    var configuration = self.defaultConfiguration
+    configuration.connectionBackoff = ConnectionBackoff()
+    let manager = self.makeConnectionManager(
+      configuration: configuration
+    ) { connectionManager, loop in
+      let channel = EmbeddedChannel(loop: loop as! EmbeddedEventLoop)
+      let multiplexer = HTTP2StreamMultiplexer(mode: .client, channel: channel) {
+        $0.eventLoop.makeSucceededVoidFuture()
+      }
+
+      let idleHandler = GRPCIdleHandler(
+        connectionManager: connectionManager,
+        multiplexer: multiplexer,
+        idleTimeout: .minutes(60),
+        keepalive: .init(),
+        logger: self.clientLogger
+      )
+
+      channel.pipeline.addHandler(idleHandler).whenSuccess {
+        escapedChannelPromise.succeed(channel)
+      }
+
+      return channelPromise.futureResult
+    }
+
+    // Ask for the multiplexer to trigger channel creation.
+    self.waitForStateChange(from: .idle, to: .connecting) {
+      _ = manager.getHTTP2Multiplexer()
+      self.loop.run()
+    }
+
+    // Fire an error down the pipeline.
+    let channel = try escapedChannelPromise.futureResult.wait()
+    channel.pipeline.fireErrorCaught(GRPCStatus(code: .unavailable))
+
+    // Fail the channel promise.
+    channelPromise.fail(GRPCStatus(code: .unavailable))
+  }
+
   func testClientKeepaliveJitterWithoutClamping() {
     let original = ClientConnectionKeepalive(interval: .seconds(2), timeout: .seconds(1))
     let keepalive = original.jitteringInterval(byAtMost: .milliseconds(500))