Fix `EventLoopFuture` and `EventLoopPromise` under strict concurrency checking #2654

FranzBusch · 2024-02-16T16:01:12Z

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 #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, I came to the conclusion that the previous @unchecked Sendable annotations were correct. The reasoning for this is:

An EventLoopPromise and EventLoopFuture pair are tied to a specific EventLoop
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
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.
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() or whenComplete()
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.

glbrntt

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)

Sources/NIOCore/EventLoop.swift

glbrntt · 2024-02-19T14:19:57Z

Sources/NIOCore/EventLoopFuture.swift

+    public func succeed(eventLoopBoundValue: Value) {
+        self._resolve(eventLoopBoundResult: .success(eventLoopBoundValue))
+    }


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.

I agree. I removed those loopBound APIs from this PR since they are really part of the AssumeIsolated PR

Sources/NIOCore/EventLoopFuture.swift

@weissi

… 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.

glbrntt

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.

dnadoba · 2024-06-27T07:37:18Z

Sources/NIOCore/EventLoopFuture.swift

+// 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 { }


No need for unchecked. EventLoopFuture is already Sendable and that is the only store property.

Suggested change

extension EventLoopPromise: @unchecked Sendable { }

extension EventLoopPromise: Sendable { }

I think this still applies.

…sendable

Lukasa · 2024-10-17T16:03:45Z

I've modified this further, and also added some narrative documentation explaining the thinking behind the type. Please re-review.

FranzBusch

LGTM. Thanks for taking the time to write the document. I can't approve the PR since I am the author but consider this comment as an approval. I only had three small comments.

Sources/NIOCore/Docs.docc/loops-futures-concurrency.md

Sources/NIOCore/EventLoopFuture.swift

…sendable

ktoso · 2024-10-18T10:01:15Z

Sources/NIOCore/Docs.docc/loops-futures-concurrency.md

+    are `@Sendable`. This is not a long-term position, but reflects the need to continue
+    to support Swift 5 code which requires this stricter standard. In a future release of
+    SwiftNIO we expect to relax this constraint: if you need this relaxed constraint
+    then please file an issue.


Not sure the repeated exact Note as above here isn't too confusing, maybe explicitly call out what "these" are or shorten the repeated notes?

ktoso · 2024-10-18T10:02:01Z

Sources/NIOCore/Docs.docc/loops-futures-concurrency.md

+and ``EventLoopFuture/get()``. These APIs can only safely be called when the ``EventLoopFuture``
+is carrying a `Sendable` value. This is because ``EventLoopFuture``s hold on to their value and
+can give it to other closures or other callers of `get` and `wait`. Thus, `sending` is not
+sufficient.


I think that's right, yes.

ktoso

I didn;t notice anything in the .md that would trigger spidey senses, It looks right I think.

Sources/NIOCore/AsyncAwaitSupport.swift

weissi · 2024-10-18T10:19:53Z

Sources/NIOCore/ChannelPipeline.swift

@@ -457,10 +457,10 @@ public final class ChannelPipeline: ChannelInvoker {
        let promise = self.eventLoop.makePromise(of: ChannelHandlerContext.self)

        if self.eventLoop.inEventLoop {
-            promise.completeWith(self.contextSync(handler: handler))
+            promise.assumeIsolated().completeWith(self.contextSync(handler: handler))


assumeIsolated() is a noop in release, right?

Right now it is but it needs to change. It uses asserts to check but we need to upgrade this preconditions since check at creation time is not enough similar to loop bound. Here is an example where just a precondition on creation of the assumeIsolated would fail

// On the EL let assumeIsolated = someFuture.assumeIsolated() assumeIsolated.wait() // Fine without a compiler error since it is no longer required to be Sendable. Also no runtime crash await withTaskExecutorPreference(someRandomExec) { assumeIsolated.wait() // No compiler error but runtime crash now }

@FranzBusch the line above is if self.eventLoop.inEventLoop(), so the assumeIsolated() is duplicated.

Ah sorry, I thought you were asking in a more general term. In this particular case I don't know if the compiler is able to optimise the precondition away.

No, it doesn't do that sort of thing. If we're not sure that this is a noop, we should remove it here. Already not super cheap to check it

Or do assertIsolated not preconditionIsolated

Sources/NIOCore/Docs.docc/loops-futures-concurrency.md

weissi · 2024-10-18T10:26:07Z

Sources/NIOCore/Docs.docc/loops-futures-concurrency.md

+
+A related concept to an "isolation domain" is an "executor". Again, useful reading can be found in
+[SE-0392 (Custom actor executors)](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0392-custom-actor-executors.md).
+At a high level, an executor is simply an object that is capable of executing Swift `Task`s. Executors can be


Probably a nit but I'm not sure if Task is 100% correct here. Like there's no way to retrieve a let theTask: Task from a child task. But then, child tasks still ask Task.isCancelled so maybe it is correct :).

I would've said that they can run Jobs but then that's not really terminology that's actually used. Probably fine

I tried for a while to rewrite this and couldn't come up with anything I liked. If we think of anything better we can change it.

Sources/NIOCore/Docs.docc/loops-futures-concurrency.md

Sources/NIOCore/EventLoopFuture+Deprecated.swift

Sources/NIOCore/EventLoopFuture.swift

FranzBusch requested review from Lukasa, dnadoba and weissi February 16, 2024 16:01

FranzBusch force-pushed the fb-event-loop-future-sendable branch 2 times, most recently from d95beaf to b438afd Compare February 19, 2024 11:10

glbrntt requested changes Feb 19, 2024

View reviewed changes

FranzBusch force-pushed the fb-event-loop-future-sendable branch from b438afd to a5c83a4 Compare February 19, 2024 15:03

George review

5d20621

FranzBusch force-pushed the fb-event-loop-future-sendable branch from a5c83a4 to 5d20621 Compare February 19, 2024 15:29

glbrntt reviewed Feb 19, 2024

View reviewed changes

Lukasa added the semver/patch No public API change. label Feb 28, 2024

Lukasa approved these changes Feb 28, 2024

View reviewed changes

Lukasa requested a review from glbrntt February 28, 2024 13:19

Merge branch 'main' into fb-event-loop-future-sendable

3ae701c

glbrntt approved these changes Mar 4, 2024

View reviewed changes

dnadoba reviewed Jun 27, 2024

View reviewed changes

Lukasa added 10 commits October 9, 2024 15:44

Merge branch 'main' into fb-event-loop-future-sendable

aeb2634

Clean up catch-up-merge issues

469e475

Merge remote-tracking branch 'origin/main' into fb-event-loop-future-…

426e453

…sendable

Pick up some missing annotations

7f83110

Merge remote-tracking branch 'origin/main' into fb-event-loop-future-…

b191621

…sendable

Formatting

b9b9ea6

Get tests passing on older Swifts

77da82c

Merge remote-tracking branch 'origin/main' into fb-event-loop-future-…

bfe84d2

…sendable

Further tweaks

75ad4d7

Add explanatory documentation

28029bf

Lukasa added 2 commits October 17, 2024 19:30

Formatting

dacc452

More formatting

c41d45d

FranzBusch commented Oct 18, 2024

View reviewed changes

Sources/NIOCore/Docs.docc/loops-futures-concurrency.md Show resolved Hide resolved

Sources/NIOCore/Docs.docc/loops-futures-concurrency.md Outdated Show resolved Hide resolved

Sources/NIOCore/EventLoopFuture.swift Show resolved Hide resolved

Lukasa added 2 commits October 18, 2024 10:07

Code review

7ce1c36

Merge remote-tracking branch 'origin/main' into fb-event-loop-future-…

d0efcf3

…sendable

ktoso reviewed Oct 18, 2024

View reviewed changes

ktoso approved these changes Oct 18, 2024

View reviewed changes

weissi reviewed Oct 18, 2024

View reviewed changes

Sources/NIOCore/EventLoopFuture+Deprecated.swift Show resolved Hide resolved

weissi reviewed Oct 18, 2024

View reviewed changes

Sources/NIOCore/EventLoopFuture.swift Outdated Show resolved Hide resolved

More code review feedback

4252ecc

Lukasa added semver/minor Adds new public API. and removed semver/patch No public API change. labels Oct 18, 2024

Lukasa merged commit ff98c93 into apple:main Oct 18, 2024
31 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `EventLoopFuture` and `EventLoopPromise` under strict concurrency checking #2654

Fix `EventLoopFuture` and `EventLoopPromise` under strict concurrency checking #2654

FranzBusch commented Feb 16, 2024

glbrntt left a comment

glbrntt Feb 19, 2024

FranzBusch Feb 19, 2024

glbrntt left a comment

dnadoba Jun 27, 2024 •

edited

Loading

FranzBusch Oct 16, 2024

Lukasa commented Oct 17, 2024

FranzBusch left a comment

ktoso Oct 18, 2024

ktoso Oct 18, 2024

ktoso left a comment

weissi Oct 18, 2024

FranzBusch Oct 18, 2024

weissi Oct 18, 2024

FranzBusch Oct 18, 2024

weissi Oct 18, 2024

weissi Oct 18, 2024

weissi Oct 18, 2024

Lukasa Oct 18, 2024

	extension EventLoopPromise: @unchecked Sendable { }
	extension EventLoopPromise: Sendable { }

Fix EventLoopFuture and EventLoopPromise under strict concurrency checking #2654

Fix EventLoopFuture and EventLoopPromise under strict concurrency checking #2654

Conversation

FranzBusch commented Feb 16, 2024

Motivation

Modification

Result

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment

dnadoba Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Oct 17, 2024

FranzBusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktoso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix `EventLoopFuture` and `EventLoopPromise` under strict concurrency checking #2654

Fix `EventLoopFuture` and `EventLoopPromise` under strict concurrency checking #2654

dnadoba Jun 27, 2024 •

edited

Loading