Skip to content

Commit 086524f

Browse files
glbrnttglbrntt
authored
Fix sendability issues in the connection pool (#833)
Motivation: The connection pool holds much of the low level logic in AHC. We should fix its sendability issues before moving to higher levels. Modifications: - Make HTTP1ConnectionDelegate and HTTP2Delegate sendable, this requires passing IDs rather than connections to their methods - Make HTTPConnectionRequester sendable and have its methods take Sendable views of the HTTP1Connection and HTTP2Connection types - Add sendable views to HTTP1Connection and HTTP2Connection - Mark HTTP1Connection and HTTP2Connection as not sendable - Make HTTPRequestExecutor and HTTPExecutableRequest sendable - Update tests Result: Connection pool has stricter sendability requirements
1 parent 0e715a2 commit 086524f

16 files changed

+459
-355
lines changed

‎.github/workflows/main.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ jobs:
1212
uses: apple/swift-nio/.github/workflows/unit_tests.yml@main
1313
with:
1414
linux_5_9_enabled: false
15-
linux_5_10_arguments_override: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error"
15+
linux_5_10_arguments_override: "--explicit-target-dependency-import-check error"
1616
linux_6_0_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"
1717
linux_6_1_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"
1818
linux_nightly_next_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"

‎.github/workflows/pull_request.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ jobs:
1515
uses: apple/swift-nio/.github/workflows/unit_tests.yml@main
1616
with:
1717
linux_5_9_enabled: false
18-
linux_5_10_arguments_override: "-Xswiftc -warnings-as-errors --explicit-target-dependency-import-check error"
18+
linux_5_10_arguments_override: "--explicit-target-dependency-import-check error"
1919
linux_6_0_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"
2020
linux_6_1_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"
2121
linux_nightly_next_arguments_override: "--explicit-target-dependency-import-check error -Xswiftc -require-explicit-sendable"

‎Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift‎

Lines changed: 38 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
185185
self.runTimeoutAction(timeoutAction, context: context)
186186
}
187187

188-
req.willExecuteRequest(self)
188+
req.willExecuteRequest(self.requestExecutor)
189189

190190
let action = self.state.runNewRequest(
191191
head: req.requestHead,
@@ -323,7 +323,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
323323
case .sendRequestEnd(let writePromise, let shouldClose):
324324
let writePromise = writePromise ?? context.eventLoop.makePromise(of: Void.self)
325325
// We need to defer succeeding the old request to avoid ordering issues
326-
writePromise.futureResult.hop(to: context.eventLoop).whenComplete { result in
326+
writePromise.futureResult.hop(to: context.eventLoop).assumeIsolated().whenComplete { result in
327327
switch result {
328328
case .success:
329329
// If our final action was `sendRequestEnd`, that means we've already received
@@ -396,7 +396,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
396396
assert(self.idleReadTimeoutTimer == nil, "Expected there is no timeout timer so far.")
397397

398398
let timerID = self.currentIdleReadTimeoutTimerID
399-
self.idleReadTimeoutTimer = self.eventLoop.scheduleTask(in: timeAmount) {
399+
self.idleReadTimeoutTimer = self.eventLoop.assumeIsolated().scheduleTask(in: timeAmount) {
400400
guard self.currentIdleReadTimeoutTimerID == timerID else { return }
401401
let action = self.state.idleReadTimeoutTriggered()
402402
self.run(action, context: context)
@@ -409,7 +409,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
409409

410410
self.currentIdleReadTimeoutTimerID &+= 1
411411
let timerID = self.currentIdleReadTimeoutTimerID
412-
self.idleReadTimeoutTimer = self.eventLoop.scheduleTask(in: timeAmount) {
412+
self.idleReadTimeoutTimer = self.eventLoop.assumeIsolated().scheduleTask(in: timeAmount) {
413413
guard self.currentIdleReadTimeoutTimerID == timerID else { return }
414414
let action = self.state.idleReadTimeoutTriggered()
415415
self.run(action, context: context)
@@ -431,7 +431,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
431431
assert(self.idleWriteTimeoutTimer == nil, "Expected there is no timeout timer so far.")
432432

433433
let timerID = self.currentIdleWriteTimeoutTimerID
434-
self.idleWriteTimeoutTimer = self.eventLoop.scheduleTask(in: timeAmount) {
434+
self.idleWriteTimeoutTimer = self.eventLoop.assumeIsolated().scheduleTask(in: timeAmount) {
435435
guard self.currentIdleWriteTimeoutTimerID == timerID else { return }
436436
let action = self.state.idleWriteTimeoutTriggered()
437437
self.run(action, context: context)
@@ -443,7 +443,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
443443

444444
self.currentIdleWriteTimeoutTimerID &+= 1
445445
let timerID = self.currentIdleWriteTimeoutTimerID
446-
self.idleWriteTimeoutTimer = self.eventLoop.scheduleTask(in: timeAmount) {
446+
self.idleWriteTimeoutTimer = self.eventLoop.assumeIsolated().scheduleTask(in: timeAmount) {
447447
guard self.currentIdleWriteTimeoutTimerID == timerID else { return }
448448
let action = self.state.idleWriteTimeoutTriggered()
449449
self.run(action, context: context)
@@ -461,8 +461,11 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
461461

462462
// MARK: Private HTTPRequestExecutor
463463

464-
private func writeRequestBodyPart0(_ data: IOData, request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?)
465-
{
464+
fileprivate func writeRequestBodyPart0(
465+
_ data: IOData,
466+
request: HTTPExecutableRequest,
467+
promise: EventLoopPromise<Void>?
468+
) {
466469
guard self.request === request, let context = self.channelContext else {
467470
// Because the HTTPExecutableRequest may run in a different thread to our eventLoop,
468471
// calls from the HTTPExecutableRequest to our ChannelHandler may arrive here after
@@ -481,7 +484,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
481484
self.run(action, context: context)
482485
}
483486

484-
private func finishRequestBodyStream0(_ request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
487+
fileprivate func finishRequestBodyStream0(_ request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
485488
guard self.request === request, let context = self.channelContext else {
486489
// See code comment in `writeRequestBodyPart0`
487490
promise?.fail(HTTPClientError.requestStreamCancelled)
@@ -492,7 +495,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
492495
self.run(action, context: context)
493496
}
494497

495-
private func demandResponseBodyStream0(_ request: HTTPExecutableRequest) {
498+
fileprivate func demandResponseBodyStream0(_ request: HTTPExecutableRequest) {
496499
guard self.request === request, let context = self.channelContext else {
497500
// See code comment in `writeRequestBodyPart0`
498501
return
@@ -504,7 +507,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
504507
self.run(action, context: context)
505508
}
506509

507-
private func cancelRequest0(_ request: HTTPExecutableRequest) {
510+
fileprivate func cancelRequest0(_ request: HTTPExecutableRequest) {
508511
guard self.request === request, let context = self.channelContext else {
509512
// See code comment in `writeRequestBodyPart0`
510513
return
@@ -524,43 +527,39 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
524527
@available(*, unavailable)
525528
extension HTTP1ClientChannelHandler: Sendable {}
526529

527-
extension HTTP1ClientChannelHandler: HTTPRequestExecutor {
528-
func writeRequestBodyPart(_ data: IOData, request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
529-
if self.eventLoop.inEventLoop {
530-
self.writeRequestBodyPart0(data, request: request, promise: promise)
531-
} else {
532-
self.eventLoop.execute {
533-
self.writeRequestBodyPart0(data, request: request, promise: promise)
530+
extension HTTP1ClientChannelHandler {
531+
var requestExecutor: RequestExecutor {
532+
RequestExecutor(self)
533+
}
534+
535+
struct RequestExecutor: HTTPRequestExecutor, Sendable {
536+
private let loopBound: NIOLoopBound<HTTP1ClientChannelHandler>
537+
538+
init(_ handler: HTTP1ClientChannelHandler) {
539+
self.loopBound = NIOLoopBound(handler, eventLoop: handler.eventLoop)
540+
}
541+
542+
func writeRequestBodyPart(_ data: IOData, request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
543+
self.loopBound.execute {
544+
$0.writeRequestBodyPart0(data, request: request, promise: promise)
534545
}
535546
}
536-
}
537547

538-
func finishRequestBodyStream(_ request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
539-
if self.eventLoop.inEventLoop {
540-
self.finishRequestBodyStream0(request, promise: promise)
541-
} else {
542-
self.eventLoop.execute {
543-
self.finishRequestBodyStream0(request, promise: promise)
548+
func finishRequestBodyStream(_ request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
549+
self.loopBound.execute {
550+
$0.finishRequestBodyStream0(request, promise: promise)
544551
}
545552
}
546-
}
547553

548-
func demandResponseBodyStream(_ request: HTTPExecutableRequest) {
549-
if self.eventLoop.inEventLoop {
550-
self.demandResponseBodyStream0(request)
551-
} else {
552-
self.eventLoop.execute {
553-
self.demandResponseBodyStream0(request)
554+
func demandResponseBodyStream(_ request: HTTPExecutableRequest) {
555+
self.loopBound.execute {
556+
$0.demandResponseBodyStream0(request)
554557
}
555558
}
556-
}
557559

558-
func cancelRequest(_ request: HTTPExecutableRequest) {
559-
if self.eventLoop.inEventLoop {
560-
self.cancelRequest0(request)
561-
} else {
562-
self.eventLoop.execute {
563-
self.cancelRequest0(request)
560+
func cancelRequest(_ request: HTTPExecutableRequest) {
561+
self.loopBound.execute {
562+
$0.cancelRequest0(request)
564563
}
565564
}
566565
}

‎Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1Connection.swift‎

Lines changed: 40 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,9 @@ import NIOCore
1717
import NIOHTTP1
1818
import NIOHTTPCompression
1919

20-
protocol HTTP1ConnectionDelegate {
21-
func http1ConnectionReleased(_: HTTP1Connection)
22-
func http1ConnectionClosed(_: HTTP1Connection)
20+
protocol HTTP1ConnectionDelegate: Sendable {
21+
func http1ConnectionReleased(_: HTTPConnectionPool.Connection.ID)
22+
func http1ConnectionClosed(_: HTTPConnectionPool.Connection.ID)
2323
}
2424

2525
final class HTTP1Connection {
@@ -67,40 +67,53 @@ final class HTTP1Connection {
6767
return connection
6868
}
6969

70-
func executeRequest(_ request: HTTPExecutableRequest) {
71-
if self.channel.eventLoop.inEventLoop {
72-
self.execute0(request: request)
73-
} else {
74-
self.channel.eventLoop.execute {
75-
self.execute0(request: request)
70+
var sendableView: SendableView {
71+
SendableView(self)
72+
}
73+
74+
struct SendableView: Sendable {
75+
private let connection: NIOLoopBound<HTTP1Connection>
76+
let channel: Channel
77+
let id: HTTPConnectionPool.Connection.ID
78+
private var eventLoop: EventLoop { self.connection.eventLoop }
79+
80+
init(_ connection: HTTP1Connection) {
81+
self.connection = NIOLoopBound(connection, eventLoop: connection.channel.eventLoop)
82+
self.id = connection.id
83+
self.channel = connection.channel
84+
}
85+
86+
func executeRequest(_ request: HTTPExecutableRequest) {
87+
self.connection.execute {
88+
$0.execute0(request: request)
7689
}
7790
}
78-
}
7991

80-
func shutdown() {
81-
self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
82-
}
92+
func shutdown() {
93+
self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
94+
}
8395

84-
func close(promise: EventLoopPromise<Void>?) {
85-
self.channel.close(mode: .all, promise: promise)
86-
}
96+
func close(promise: EventLoopPromise<Void>?) {
97+
self.channel.close(mode: .all, promise: promise)
98+
}
8799

88-
func close() -> EventLoopFuture<Void> {
89-
let promise = self.channel.eventLoop.makePromise(of: Void.self)
90-
self.close(promise: promise)
91-
return promise.futureResult
100+
func close() -> EventLoopFuture<Void> {
101+
let promise = self.eventLoop.makePromise(of: Void.self)
102+
self.close(promise: promise)
103+
return promise.futureResult
104+
}
92105
}
93106

94107
func taskCompleted() {
95-
self.delegate.http1ConnectionReleased(self)
108+
self.delegate.http1ConnectionReleased(self.id)
96109
}
97110

98111
private func execute0(request: HTTPExecutableRequest) {
99112
guard self.channel.isActive else {
100113
return request.fail(ChannelError.ioOnClosedChannel)
101114
}
102115

103-
self.channel.write(request, promise: nil)
116+
self.channel.pipeline.syncOperations.write(NIOAny(request), promise: nil)
104117
}
105118

106119
private func start(decompression: HTTPClient.Decompression, logger: Logger) throws {
@@ -111,9 +124,9 @@ final class HTTP1Connection {
111124
}
112125

113126
self.state = .active
114-
self.channel.closeFuture.whenComplete { _ in
127+
self.channel.closeFuture.assumeIsolated().whenComplete { _ in
115128
self.state = .closed
116-
self.delegate.http1ConnectionClosed(self)
129+
self.delegate.http1ConnectionClosed(self.id)
117130
}
118131

119132
do {
@@ -150,3 +163,6 @@ final class HTTP1Connection {
150163
}
151164
}
152165
}
166+
167+
@available(*, unavailable)
168+
extension HTTP1Connection: Sendable {}

‎Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift‎

Lines changed: 29 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
137137
self.runTimeoutAction(timeoutAction, context: context)
138138
}
139139

140-
request.willExecuteRequest(self)
140+
request.willExecuteRequest(self.requestExecutor)
141141

142142
let action = self.state.startRequest(
143143
head: request.requestHead,
@@ -313,7 +313,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
313313
assert(self.idleReadTimeoutTimer == nil, "Expected there is no timeout timer so far.")
314314

315315
let timerID = self.currentIdleReadTimeoutTimerID
316-
self.idleReadTimeoutTimer = self.eventLoop.scheduleTask(in: timeAmount) {
316+
self.idleReadTimeoutTimer = self.eventLoop.assumeIsolated().scheduleTask(in: timeAmount) {
317317
guard self.currentIdleReadTimeoutTimerID == timerID else { return }
318318
let action = self.state.idleReadTimeoutTriggered()
319319
self.run(action, context: context)
@@ -326,7 +326,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
326326

327327
self.currentIdleReadTimeoutTimerID &+= 1
328328
let timerID = self.currentIdleReadTimeoutTimerID
329-
self.idleReadTimeoutTimer = self.eventLoop.scheduleTask(in: timeAmount) {
329+
self.idleReadTimeoutTimer = self.eventLoop.assumeIsolated().scheduleTask(in: timeAmount) {
330330
guard self.currentIdleReadTimeoutTimerID == timerID else { return }
331331
let action = self.state.idleReadTimeoutTriggered()
332332
self.run(action, context: context)
@@ -349,7 +349,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
349349
assert(self.idleWriteTimeoutTimer == nil, "Expected there is no timeout timer so far.")
350350

351351
let timerID = self.currentIdleWriteTimeoutTimerID
352-
self.idleWriteTimeoutTimer = self.eventLoop.scheduleTask(in: timeAmount) {
352+
self.idleWriteTimeoutTimer = self.eventLoop.assumeIsolated().scheduleTask(in: timeAmount) {
353353
guard self.currentIdleWriteTimeoutTimerID == timerID else { return }
354354
let action = self.state.idleWriteTimeoutTriggered()
355355
self.run(action, context: context)
@@ -361,7 +361,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
361361

362362
self.currentIdleWriteTimeoutTimerID &+= 1
363363
let timerID = self.currentIdleWriteTimeoutTimerID
364-
self.idleWriteTimeoutTimer = self.eventLoop.scheduleTask(in: timeAmount) {
364+
self.idleWriteTimeoutTimer = self.eventLoop.assumeIsolated().scheduleTask(in: timeAmount) {
365365
guard self.currentIdleWriteTimeoutTimerID == timerID else { return }
366366
let action = self.state.idleWriteTimeoutTriggered()
367367
self.run(action, context: context)
@@ -437,43 +437,39 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
437437
@available(*, unavailable)
438438
extension HTTP2ClientRequestHandler: Sendable {}
439439

440-
extension HTTP2ClientRequestHandler: HTTPRequestExecutor {
441-
func writeRequestBodyPart(_ data: IOData, request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
442-
if self.eventLoop.inEventLoop {
443-
self.writeRequestBodyPart0(data, request: request, promise: promise)
444-
} else {
445-
self.eventLoop.execute {
446-
self.writeRequestBodyPart0(data, request: request, promise: promise)
440+
extension HTTP2ClientRequestHandler {
441+
var requestExecutor: RequestExecutor {
442+
RequestExecutor(self)
443+
}
444+
445+
struct RequestExecutor: HTTPRequestExecutor, Sendable {
446+
private let loopBound: NIOLoopBound<HTTP2ClientRequestHandler>
447+
448+
init(_ handler: HTTP2ClientRequestHandler) {
449+
self.loopBound = NIOLoopBound(handler, eventLoop: handler.eventLoop)
450+
}
451+
452+
func writeRequestBodyPart(_ data: IOData, request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
453+
self.loopBound.execute {
454+
$0.writeRequestBodyPart0(data, request: request, promise: promise)
447455
}
448456
}
449-
}
450457

451-
func finishRequestBodyStream(_ request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
452-
if self.eventLoop.inEventLoop {
453-
self.finishRequestBodyStream0(request, promise: promise)
454-
} else {
455-
self.eventLoop.execute {
456-
self.finishRequestBodyStream0(request, promise: promise)
458+
func finishRequestBodyStream(_ request: HTTPExecutableRequest, promise: EventLoopPromise<Void>?) {
459+
self.loopBound.execute {
460+
$0.finishRequestBodyStream0(request, promise: promise)
457461
}
458462
}
459-
}
460463

461-
func demandResponseBodyStream(_ request: HTTPExecutableRequest) {
462-
if self.eventLoop.inEventLoop {
463-
self.demandResponseBodyStream0(request)
464-
} else {
465-
self.eventLoop.execute {
466-
self.demandResponseBodyStream0(request)
464+
func demandResponseBodyStream(_ request: HTTPExecutableRequest) {
465+
self.loopBound.execute {
466+
$0.demandResponseBodyStream0(request)
467467
}
468468
}
469-
}
470469

471-
func cancelRequest(_ request: HTTPExecutableRequest) {
472-
if self.eventLoop.inEventLoop {
473-
self.cancelRequest0(request)
474-
} else {
475-
self.eventLoop.execute {
476-
self.cancelRequest0(request)
470+
func cancelRequest(_ request: HTTPExecutableRequest) {
471+
self.loopBound.execute {
472+
$0.cancelRequest0(request)
477473
}
478474
}
479475
}

0 commit comments

Comments
 (0)