Browse Source

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.
George Barnett 2 years ago
parent
commit
4f8cbe0132
2 changed files with 50 additions and 2 deletions
  1. 3 1
      Sources/GRPC/ConnectionManager.swift
  2. 47 1
      Tests/GRPCTests/ConnectionManagerTests.swift

+ 3 - 1
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

+ 47 - 1
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))