-
Notifications
You must be signed in to change notification settings - Fork 648
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
Fix EventLoopFuture
and EventLoopPromise
under strict concurrency checking
#2654
base: main
Are you sure you want to change the base?
Conversation
d95beaf
to
b438afd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the explanation makes sense and this all ties together nicely. I think the spelling of the new APIs is not quite right though and we should align with patterns we've used elsewhere in NIO (i.e. sync views)
public func succeed(eventLoopBoundValue: Value) { | ||
self._resolve(eventLoopBoundResult: .success(eventLoopBoundValue)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the right spelling; the pattern we have for functions which must be called on the right EL with runtime assertions are synchronous views (e.g. ChannelPipeline.SynchronousOperations
and Channel.syncOptions
).
It also skirts around the slight oddity with this API in that you could call this function with a Sendable
i.e. not EL bound value with no ill side effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I removed those loopBound
APIs from this PR since they are really part of the AssumeIsolated
PR
… checking # Motivation We need to tackle the remaining strict concurrency checking related `Sendable` warnings in NIO. The first place to start is making sure that `EventLoopFuture` and `EventLoopPromise` are properly annotated. # Modification In a previous apple#2496, @weissi changed the `@unchecked Sendable` conformances of `EventLoopFuture/Promise` to be conditional on the sendability of the generic `Value` type. After having looked at all the APIs on the future and promise types as well as reading the latest Concurrency evolution proposals, specifically the [Region based Isolation](https://github.com/apple/swift-evolution/blob/main/proposals/0414-region-based-isolation.md), I came to the conclusion that the previous `@unchecked Sendable` annotations were correct. The reasoning for this is: 1. An `EventLoopPromise` and `EventLoopFuture` pair are tied to a specific `EventLoop` 2. An `EventLoop` represents an isolation region and values tied to its isolation are not allowed to be shared outside of it unless they are disconnected from the region 3. The `value` used to succeed a promise often come from outside the isolation domain of the `EventLoop` hence they must be transferred into the promise. 4. The isolation region of the event loop is enforced through `@Sendable` annotations on all closures that receive the value in some kind of transformation e.g. `map()` 5. Any method on `EventLoopFuture` that combines itself with another future must require `Sendable` of the other futures `Value` since we cannot statically enforce that futures are bound to the same event loop i.e. to the same isolation domain Due to the above rules, this PR adds back the `@unchecked Sendable` conformances to both types. Furthermore, this PR revisits every single method on `EventLoopPromise/Future` and adds missing `Sendable` and `@Sendable` annotation where necessary to uphold the above rules. A few important things to call out: - Since `transferring` is currently not available this PR requires a `Sendable` conformance for some methods on `EventLoopPromise/Future` that should rather take a `transffering` argument - To enable the common case where a value from the same event loop is used to succeed a promise I added two additional methods that take a `eventLoopBoundResult` and enforce dynamic isolation checking. We might have to do this for more methods once we adopt those changes in other targets/packages. # Result After this PR has landed our lowest level building block should be inline with what the rest of the language enforces in Concurrency. The `EventLoopFuture.swift` produces no more warnings under strict concurrency checking on the latest 5.10 snapshots.
b438afd
to
a5c83a4
Compare
a5c83a4
to
5d20621
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now. Purposefully not selecting 'Approve' as I'd like at least one other set of eyes to approve this too, but consider it approved by me.
// transfer the value to the promise. This ensures that the value is now isolated to the event loops | ||
// isolation domain. Note: Sendable values can always be transferred | ||
|
||
extension EventLoopPromise: @unchecked Sendable { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non need for unchecked. EventLoopFuture
is already Sendable
and that is the only store property.
extension EventLoopPromise: @unchecked Sendable { } | |
extension EventLoopPromise: Sendable { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still applies.
I've modified this further, and also added some narrative documentation explaining the thinking behind the type. Please re-review. |
Motivation
We need to tackle the remaining strict concurrency checking related
Sendable
warnings in NIO. The first place to start is making sure thatEventLoopFuture
andEventLoopPromise
are properly annotated.Modification
In a previous #2496, @weissi changed the
@unchecked Sendable
conformances ofEventLoopFuture/Promise
to be conditional on the sendability of the genericValue
type. After having looked at all the APIs on the future and promise types as well as reading the latest Concurrency evolution proposals, specifically the Region based Isolation, I came to the conclusion that the previous@unchecked Sendable
annotations were correct. The reasoning for this is:EventLoopPromise
andEventLoopFuture
pair are tied to a specificEventLoop
EventLoop
represents an isolation region and values tied to its isolation are not allowed to be shared outside of it unless they are disconnected from the regionvalue
used to succeed a promise often come from outside the isolation domain of theEventLoop
hence they must be transferred into the promise.@Sendable
annotations on all closures that receive the value in some kind of transformation e.g.map()
orwhenComplete()
EventLoopFuture
that combines itself with another future must requireSendable
of the other futuresValue
since we cannot statically enforce that futures are bound to the same event loop i.e. to the same isolation domainDue to the above rules, this PR adds back the
@unchecked Sendable
conformances to both types. Furthermore, this PR revisits every single method onEventLoopPromise/Future
and adds missingSendable
and@Sendable
annotation where necessary to uphold the above rules. A few important things to call out:transferring
is currently not available this PR requires aSendable
conformance for some methods onEventLoopPromise/Future
that should rather take atransffering
argumenteventLoopBoundResult
and enforce dynamic isolation checking. We might have to do this for more methods once we adopt those changes in other targets/packages.Result
After this PR has landed our lowest level building block should be inline with what the rest of the language enforces in Concurrency. The
EventLoopFuture.swift
produces no more warnings under strict concurrency checking on the latest 5.10 snapshots.