Use configured connect timeout when retries is none (#1777)

Motivation:

The connection manager uses an iterator to get values for the connect
timeout and the backoff to use if the connection attempt fails. Backoff
can also be configured to have a limited number of tries. This controls
when the iterator returns `nil`. If the connection attempt retries are
disabled then the iterator always returns `nil`. This is an issue
because the connection manager doesn't respect the configured connect
timeout as the iterator doesn't return a value.

Modifications:

Check when starting a connection attempt from the idle state whether
there a connect timeout is configured if there are no retries. This

Result:

The connect timeout value is repected if the connection retries aren't
enabled.

Author: glbrntt

Sources/GRPC/ConnectionManager.swift

@@ -1011,9 +1011,22 @@ extension ConnectionManager {
     switch self.state {
     case .idle:
       let iterator = self.connectionBackoff?.makeIterator()
+
+      // The iterator produces the connect timeout and the backoff to use for the next attempt. This
+      // is unfortunate if retries is set to none because we need to connect timeout but not the
+      // backoff yet the iterator will not return a value to us. To workaround this we grab the
+      // connect timeout and override it.
+      let connectTimeoutOverride: TimeAmount?
+      if let backoff = self.connectionBackoff, backoff.retries == .none {
+        connectTimeoutOverride = .seconds(timeInterval: backoff.minimumConnectionTimeout)
+      } else {
+        connectTimeoutOverride = nil
+      }
+
       self.startConnecting(
         backoffIterator: iterator,
-        muxPromise: self.eventLoop.makePromise()
+        muxPromise: self.eventLoop.makePromise(),
+        connectTimeoutOverride: connectTimeoutOverride
       )
 
     case let .transientFailure(pending):
@@ -1039,7 +1052,8 @@ extension ConnectionManager {
 
   private func startConnecting(
     backoffIterator: ConnectionBackoffIterator?,
-    muxPromise: EventLoopPromise<HTTP2StreamMultiplexer>
+    muxPromise: EventLoopPromise<HTTP2StreamMultiplexer>,
+    connectTimeoutOverride: TimeAmount? = nil
   ) {
     let timeoutAndBackoff = backoffIterator?.next()
 
@@ -1048,10 +1062,17 @@ extension ConnectionManager {
     self.eventLoop.assertInEventLoop()
 
     let candidate: EventLoopFuture<Channel> = self.eventLoop.flatSubmit {
+      let connectTimeout: TimeAmount?
+      if let connectTimeoutOverride = connectTimeoutOverride {
+        connectTimeout = connectTimeoutOverride
+      } else {
+        connectTimeout = timeoutAndBackoff.map { TimeAmount.seconds(timeInterval: $0.timeout) }
+      }
+
       let channel: EventLoopFuture<Channel> = self.channelProvider.makeChannel(
         managedBy: self,
         onEventLoop: self.eventLoop,
-        connectTimeout: timeoutAndBackoff.map { .seconds(timeInterval: $0.timeout) },
+        connectTimeout: connectTimeout,
         logger: self.logger
       )
 

Tests/GRPCTests/ConnectionManagerTests.swift

@@ -1357,6 +1357,51 @@ extension ConnectionManagerTests {
     XCTAssertGreaterThan(keepalive.interval, .seconds(1))
     XCTAssertLessThanOrEqual(keepalive.interval, .seconds(4))
   }
+
+  func testConnectTimeoutIsRespectedWithNoRetries() {
+    // Setup a factory which makes channels. We'll use this as the point to check that the
+    // connect timeout is as expected.
+    struct Provider: ConnectionManagerChannelProvider {
+      func makeChannel(
+        managedBy connectionManager: ConnectionManager,
+        onEventLoop eventLoop: any EventLoop,
+        connectTimeout: TimeAmount?,
+        logger: Logger
+      ) -> EventLoopFuture<Channel> {
+        XCTAssertEqual(connectTimeout, .seconds(314_159_265))
+        return eventLoop.makeFailedFuture(DoomedChannelError())
+      }
+    }
+
+    var configuration = self.defaultConfiguration
+    configuration.connectionBackoff = ConnectionBackoff(
+      minimumConnectionTimeout: 314_159_265,
+      retries: .none
+    )
+
+    let manager = ConnectionManager(
+      configuration: configuration,
+      channelProvider: Provider(),
+      connectivityDelegate: self.monitor,
+      logger: self.logger
+    )
+
+    // Setup the state change expectations and trigger them by asking for the multiplexer.
+    // We expect connecting to shutdown as no connect retries are configured and the factory
+    // always returns errors.
+    let multiplexer = self.waitForStateChanges([
+      Change(from: .idle, to: .connecting),
+      Change(from: .connecting, to: .shutdown),
+    ]) {
+      let multiplexer = manager.getHTTP2Multiplexer()
+      self.loop.run()
+      return multiplexer
+    }
+
+    XCTAssertThrowsError(try multiplexer.wait()) { error in
+      XCTAssert(error is DoomedChannelError)
+    }
+  }
 }
 
 internal struct Change: Hashable, CustomStringConvertible {