Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use configured connect timeout when retries is none #1777

Merged
merged 2 commits into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions Sources/GRPC/ConnectionManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -1039,7 +1052,8 @@ extension ConnectionManager {

private func startConnecting(
backoffIterator: ConnectionBackoffIterator?,
muxPromise: EventLoopPromise<HTTP2StreamMultiplexer>
muxPromise: EventLoopPromise<HTTP2StreamMultiplexer>,
connectTimeoutOverride: TimeAmount? = nil
) {
let timeoutAndBackoff = backoffIterator?.next()

Expand All @@ -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
)

Expand Down
45 changes: 45 additions & 0 deletions Tests/GRPCTests/ConnectionManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading