Strict concurrency for NIOTransportServices and tests #228
Conversation
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Thanks Gus, this mostly looks great.
Package.swift
Outdated
| @@ -1,4 +1,4 @@ | |||
| // swift-tools-version:5.8 | |||
| // swift-tools-version:5.9 | |||
There was a problem hiding this comment.
Can you drop 5.8 in a separate PR please?
Package.swift
Outdated
|
|
||
| import PackageDescription | ||
|
|
||
| let strictConcurrencyDevelopment = true |
| let childChannelOptions = self.childChannelOptions | ||
|
|
||
| @inline(__always) | ||
| @preconcurrency @Sendable @inline(__always) |
There was a problem hiding this comment.
Why do we need @preconcurrency here?
There was a problem hiding this comment.
We don't, not sure why I added this
Sources/NIOTransportServices/Datagram/NIOTSDatagramBootstrap.swift
Outdated
Show resolved
Hide resolved
| private let readPromise: NIOLockedValueBox<EventLoopPromise<Void>?> | ||
| private let cumulationBuffer: NIOLockedValueBox<ByteBuffer?> |
There was a problem hiding this comment.
Not a big deal but I would bias away from using multiple locks (easy to run into ordering issues where you deadlock). In this instance I don't think I'd even make the class Sendable, I'd just pass the optional promise to the init. cumulationBuffer is isolated to the event-loop anyway.
There was a problem hiding this comment.
It needs to be Sendable because we use it in Channel/expectRead(_:) (defined in this file too) and we're not using an embedded EL so I don't think we can assume isolated anywhere.
There was a problem hiding this comment.
You don't need to use assume isolated here; you can create the promise off of the event-loop then create the handler and init it with the promise on the right event-loop. I.e. only the promise needs to be sendable here.
|
|
||
| #if canImport(Network) | ||
| import Dispatch | ||
| @preconcurrency import Dispatch |
There was a problem hiding this comment.
I'm a bit suspicious of this, why do we need it? I thought Dispatch had adopted Sendable etc.
There was a problem hiding this comment.
The macOS CI (that I added temporarily in a commit to check it all built properly) failed without @preconcurrency.
There was a problem hiding this comment.
Any idea why/where it failed?
There was a problem hiding this comment.
/Users/github/actions-runner/_work/swift-nio-transport-services/swift-nio-transport-services/Sources/NIOTransportServices/NIOTSEventLoop.swift:207:13: error: capture of 'timerSource' with non-sendable type 'any DispatchSourceTimer' in a `@Sendable` closure; this is an error in the Swift 6 language mode
timerSource.cancel()
^
Dispatch.DispatchSourceTimer:1:17: note: protocol 'DispatchSourceTimer' does not conform to the 'Sendable' protocol
public protocol DispatchSourceTimer : DispatchSourceProtocol {
^
/Users/github/actions-runner/_work/swift-nio-transport-services/swift-nio-transport-services/Sources/NIOTransportServices/NIOTSEventLoop.swift:16:1: error: add '@preconcurrency' to suppress 'Sendable'-related warnings from module 'Dispatch'
import Dispatch
^
@preconcurrency
/Users/github/actions-runner/_work/swift-nio-transport-services/swift-nio-transport-services/Sources/NIOTransportServices/NIOTSEventLoop.swift:213:17: error: capture of 'timerSource' with non-sendable type 'any DispatchSourceTimer' in a `@Sendable` closure; this is an error in the Swift 6 language mode
timerSource.cancel()
^
Dispatch.DispatchSourceTimer:1:17: note: protocol 'DispatchSourceTimer' does not conform to the 'Sendable' protocol
public protocol DispatchSourceTimer : DispatchSourceProtocol {
^
Command SwiftCompile failed with a nonzero exit code
Xcode 16.0, 16.1, 16.2
There was a problem hiding this comment.
DispatchSourceTimer is Sendable according to https://developer.apple.com/documentation/dispatch/dispatchsourcetimer
It's not clear when it was made Sendable but I think it's worth digging into this some more as I don't think we should import this with a catch-all @preconcurrency import.
There was a problem hiding this comment.
Looks like DispatchSourceTimer conforms to Sendable only from Xcode 16.3 (i.e. Swift 6.1). I've added a compiler guard to include @preconcurrency only if the Swift version is <6.1
| private var open: Bool { | ||
| self.state == .active | ||
| } | ||
| private let state = NIOLockedValueBox(LifecycleState.active(registeredChannels: [:])) |
There was a problem hiding this comment.
No for LifecycleState, yes for NIOTSEventLoop. IMO that's the right move though: the lock is completely superfluous here as the state is isolated to the event-loop. The lock just tells the compiler to shut up and adds overhead.
| private let readPromise: NIOLockedValueBox<EventLoopPromise<Void>?> | ||
| private let cumulationBuffer: NIOLockedValueBox<ByteBuffer?> |
There was a problem hiding this comment.
You don't need to use assume isolated here; you can create the promise off of the event-loop then create the handler and init it with the promise on the right event-loop. I.e. only the promise needs to be sendable here.
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
A couple of small things need fixing but LGTM otherwise!
| p.assumeIsolated().completeWith( | ||
| Result { | ||
| try task() | ||
| } | ||
| ) |
There was a problem hiding this comment.
Shouldn't the scheduleTask func take a Sendable closure? (The other variants do.)
There was a problem hiding this comment.
Yeah good catch - I think I also missed @preconcurrency on all these public methods where I've added @Sendable to the closures.
I think I'll still have to do the assumeIsolated dance though, because T isn't Sendable.
2 participants
No description provided.