Skip to content

Commit 2ff03e3

Browse files
committed
Avoid invalid state when a connect failed (grpc#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.
1 parent 78785d7 commit 2ff03e3

File tree

2 files changed

+50
-2
lines changed

2 files changed

+50
-2
lines changed

Sources/GRPC/ConnectionManager.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -657,7 +657,9 @@ internal final class ConnectionManager: @unchecked Sendable {
657657
)
658658

659659
case .connecting:
660-
self.connectionFailed(withError: error)
660+
// Ignore the error, the channel promise will notify the manager of any error which occurs
661+
// while connecting.
662+
()
661663

662664
case var .active(state):
663665
state.error = error

Tests/GRPCTests/ConnectionManagerTests.swift

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1272,12 +1272,58 @@ extension ConnectionManagerTests {
12721272
}
12731273

12741274
self.waitForStateChange(from: .connecting, to: .shutdown) {
1275-
manager.channelError(EventLoopError.shutdown)
1275+
channelPromise.fail(EventLoopError.shutdown)
12761276
}
12771277

12781278
XCTAssertThrowsError(try multiplexer.wait())
12791279
}
12801280

1281+
func testChannelErrorAndConnectFailWhenConnecting() throws {
1282+
// This test checks a path through the connection manager which previously led to an invalid
1283+
// state (a connect failure in a state other than connecting). To trigger these we need to
1284+
// fire an error down the pipeline containing the idle handler and fail the connect promise.
1285+
let escapedChannelPromise = self.loop.makePromise(of: Channel.self)
1286+
let channelPromise = self.loop.makePromise(of: Channel.self)
1287+
1288+
var configuration = self.defaultConfiguration
1289+
configuration.connectionBackoff = ConnectionBackoff()
1290+
let manager = self.makeConnectionManager(
1291+
configuration: configuration
1292+
) { connectionManager, loop in
1293+
let channel = EmbeddedChannel(loop: loop as! EmbeddedEventLoop)
1294+
let multiplexer = HTTP2StreamMultiplexer(mode: .client, channel: channel) {
1295+
$0.eventLoop.makeSucceededVoidFuture()
1296+
}
1297+
1298+
let idleHandler = GRPCIdleHandler(
1299+
connectionManager: connectionManager,
1300+
multiplexer: multiplexer,
1301+
idleTimeout: .minutes(60),
1302+
keepalive: .init(),
1303+
logger: self.clientLogger
1304+
)
1305+
1306+
channel.pipeline.addHandler(idleHandler).whenSuccess {
1307+
escapedChannelPromise.succeed(channel)
1308+
}
1309+
1310+
return channelPromise.futureResult
1311+
}
1312+
1313+
// Ask for the multiplexer to trigger channel creation.
1314+
self.waitForStateChange(from: .idle, to: .connecting) {
1315+
_ = manager.getHTTP2Multiplexer()
1316+
self.loop.run()
1317+
}
1318+
1319+
// Fire an error down the pipeline.
1320+
let channel = try escapedChannelPromise.futureResult.wait()
1321+
channel.pipeline.fireErrorCaught(GRPCStatus(code: .unavailable))
1322+
1323+
// Fail the channel promise.
1324+
channelPromise.fail(GRPCStatus(code: .unavailable))
1325+
}
1326+
12811327
func testClientKeepaliveJitterWithoutClamping() {
12821328
let original = ClientConnectionKeepalive(interval: .seconds(2), timeout: .seconds(1))
12831329
let keepalive = original.jitteringInterval(byAtMost: .milliseconds(500))

0 commit comments

Comments
 (0)