Fix EventLoopFuture and EventLoopPromise under strict concurrency checking (#2654)

# 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](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()` or `whenComplete()`
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.

---------

Co-authored-by: George Barnett <[email protected]>
Co-authored-by: Cory Benfield <[email protected]>

Author: 3 people

Sources/NIOCore/AsyncAwaitSupport.swift

@@ -18,8 +18,9 @@ extension EventLoopFuture {
     /// This function can be used to bridge an `EventLoopFuture` into the `async` world. Ie. if you're in an `async`
     /// function and want to get the result of this future.
     @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
+    @preconcurrency
     @inlinable
-    public func get() async throws -> Value {
+    public func get() async throws -> Value where Value: Sendable {
         try await withUnsafeThrowingContinuation { (cont: UnsafeContinuation<UnsafeTransfer<Value>, Error>) in
             self.whenComplete { result in
                 switch result {
@@ -62,8 +63,11 @@ extension EventLoopPromise {
     /// - returns: A `Task` which was created to `await` the `body`.
     @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
     @discardableResult
+    @preconcurrency
     @inlinable
-    public func completeWithTask(_ body: @escaping @Sendable () async throws -> Value) -> Task<Void, Never> {
+    public func completeWithTask(
+        _ body: @escaping @Sendable () async throws -> Value
+    ) -> Task<Void, Never> where Value: Sendable {
         Task {
             do {
                 let value = try await body()
@@ -396,8 +400,9 @@ struct AsyncSequenceFromIterator<AsyncIterator: AsyncIteratorProtocol>: AsyncSeq
 
 @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
 extension EventLoop {
+    @preconcurrency
     @inlinable
-    public func makeFutureWithTask<Return>(
+    public func makeFutureWithTask<Return: Sendable>(
         _ body: @Sendable @escaping () async throws -> Return
     ) -> EventLoopFuture<Return> {
         let promise = self.makePromise(of: Return.self)

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))
         } else {
             self.eventLoop.execute {
-                promise.completeWith(self.contextSync(handler: handler))
+                promise.assumeIsolated().completeWith(self.contextSync(handler: handler))
             }
         }
 
@@ -486,10 +486,10 @@ public final class ChannelPipeline: ChannelInvoker {
         let promise = self.eventLoop.makePromise(of: ChannelHandlerContext.self)
 
         if self.eventLoop.inEventLoop {
-            promise.completeWith(self.contextSync(name: name))
+            promise.assumeIsolated().completeWith(self.contextSync(name: name))
         } else {
             self.eventLoop.execute {
-                promise.completeWith(self.contextSync(name: name))
+                promise.assumeIsolated().completeWith(self.contextSync(name: name))
             }
         }
 
@@ -519,10 +519,10 @@ public final class ChannelPipeline: ChannelInvoker {
         let promise = self.eventLoop.makePromise(of: ChannelHandlerContext.self)
 
         if self.eventLoop.inEventLoop {
-            promise.completeWith(self._contextSync(handlerType: handlerType))
+            promise.assumeIsolated().completeWith(self._contextSync(handlerType: handlerType))
         } else {
             self.eventLoop.execute {
-                promise.completeWith(self._contextSync(handlerType: handlerType))
+                promise.assumeIsolated().completeWith(self._contextSync(handlerType: handlerType))
             }
         }
 

Sources/NIOCore/DispatchQueue+WithFuture.swift

@@ -29,9 +29,10 @@ extension DispatchQueue {
     ///     - callbackMayBlock: The scheduled callback for the IO / task.
     /// - returns a new `EventLoopFuture<ReturnType>` with value returned by the `block` parameter.
     @inlinable
-    public func asyncWithFuture<NewValue>(
+    @preconcurrency
+    public func asyncWithFuture<NewValue: Sendable>(
         eventLoop: EventLoop,
-        _ callbackMayBlock: @escaping () throws -> NewValue
+        _ callbackMayBlock: @escaping @Sendable () throws -> NewValue
     ) -> EventLoopFuture<NewValue> {
         let promise = eventLoop.makePromise(of: NewValue.self)
 

Sources/NIOCore/Docs.docc/index.md

@@ -15,6 +15,7 @@ More specialized modules provide concrete implementations of many of the abstrac
 
 - <doc:swift-concurrency>
 - <doc:ByteBuffer-lengthPrefix>
+- <doc:loops-futures-concurrency>
 
 ### Event Loops and Event Loop Groups
 

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

@@ -0,0 +1,176 @@
+# EventLoops, EventLoopFutures, and Swift Concurrency
+
+This article aims to communicate how NIO's ``EventLoop``s and ``EventLoopFuture``s interact with the Swift 6
+concurrency model, particularly regarding data-race safety. It aims to be a reference for writing correct
+concurrent code in the NIO model.
+
+NIO predates the Swift concurrency model. As a result, several of NIO's concepts are not perfect matches to
+the concepts that Swift uses, or have overlapping responsibilities.
+
+## Isolation domains and executors
+
+First, a quick recap. The core of Swift 6's data-race safety protection is the concept of an "isolation
+domain". Some valuable reading regarding the concept can be found in
+[SE-0414 (Region based isolation)](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0414-region-based-isolation.md)
+but at a high level an isolation domain can be understood to be a collection of state and methods within which there cannot be
+multiple executors executing code at the same time.
+
+In standard Swift Concurrency, the main boundaries of isolation domains are actors and tasks. Each actor,
+including global actors, defines an isolation domain. Additionally, for functions and methods that are
+not isolated to an actor, the `Task` within which that code executes defines an isolation domain. Passing
+values between these isolation domains requires that these values are either `Sendable` (safe to hold in
+multiple domains), or that the `sending` keyword is used to force the value to be passed from one domain
+to another.
+
+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
+concurrent, or they can be serial. Serial executors are the most common, as they can be used to back an
+actor.
+
+## Event Loops
+
+NIO's core execution primitive is the ``EventLoop``. An ``EventLoop`` is fundamentally nothing more than
+a Swift Concurrency Serial Executor that can also perform I/O operations directly. Indeed, NIO's
+``EventLoop``s can be exposed as serial executors, using ``EventLoop/executor``. This provides a mechanism
+to protect actor-isolated state using a NIO event-loop. With [the introduction of task executors](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0417-task-executor-preference.md),
+future versions of SwiftNIO will also be able to offer their event loops for individual `Task`s to execute
+on as well.
+
+In a Swift 6 world, it is possible that these would be the API that NIO offered to execute tasks on the
+loop. However, as NIO predates Swift 6, it also offers its own set of APIs to enqueue work. This includes
+(but is not limited to):
+
+- ``EventLoop/execute(_:)``
+- ``EventLoop/submit(_:)``
+- ``EventLoop/scheduleTask(in:_:)``
+- ``EventLoop/scheduleRepeatedTask(initialDelay:delay:notifying:_:)``
+- ``EventLoop/scheduleCallback(at:handler:)-2xm6l``
+
+The existence of these APIs requires us to also ask the question of where the submitted code executes. The
+answer is that the submitted code executes on the event loop (or, in Swift Concurrency terms, on the
+executor provided by the event loop).
+
+As the event loop only ever executes a single item of work (either an `async` function or one of the
+closures above) at a time, it is a _serial_ executor. It also provides an _isolation domain_: code
+submitted to a given `EventLoop` never runs in parallel with other code submitted to the same loop.
+
+The result here is that a all closures passed into the event loop to do work must be transferred
+in: they may not be kept hold of outside of the event loop. That means they must be sent using
+the `sending` keyword.
+
+> Note: As of the current 2.75.0 release, NIO enforces the stricter requirement that these closures
+    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.
+
+## Event loop futures
+
+In Swift NIO the most common mechanism to arrange a series of asynchronous work items is
+_not_ to queue up a series of ``EventLoop/execute(_:)`` calls. Instead, users typically
+use ``EventLoopFuture``.
+
+``EventLoopFuture`` has some extensive semantics documented in its API documentation. The
+most important principal for this discussion is that all callbacks added to an
+``EventLoopFuture`` will execute on the ``EventLoop`` to which that ``EventLoopFuture`` is
+bound. By extension, then, all callbacks added to an ``EventLoopFuture`` execute on the same
+executor (the ``EventLoop``) in the same isolation domain.
+
+The analogy to an actor here is hopefully fairly clear. Conceptually, an ``EventLoopFuture``
+could be modelled as an actor. That means all the callbacks have the same logical semantics:
+the ``EventLoopFuture`` uses the isolation domain of its associated ``EventLoop``, and all
+the callbacks are `sent` into the isolation domain. To that end, all the callback-taking APIs
+require that the callback is sent using `sending` into the ``EventLoopFuture``.
+
+> Note: As of the current 2.75.0 release, NIO enforces the stricter requirement that these callbacks
+    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.
+
+Unlike ``EventLoop``s, however, ``EventLoopFuture``s also have value-receiving and value-taking
+APIs. This is because ``EventLoopFuture``s pass a value along to their various callbacks, and
+so need to be both given an initial value (via an ``EventLoopPromise``) and in some cases to
+extract that value from the ``EventLoopFuture`` wrapper.
+
+This implies that ``EventLoopPromise``'s various success functions
+(_and_ ``EventLoop/makeSucceededFuture(_:)``) need to take their value as `sending`. The value
+is potentially sent from its current isolation domain into the ``EventLoop``, which will require
+that the value is safe to move.
+
+> Note: As of the current 2.75.0 release, NIO enforces the stricter requirement that these values
+    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.
+
+There are also a few ways to extract a value, such as ``EventLoopFuture/wait(file:line:)``
+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.
+
+## Combining Futures
+
+NIO provides a number of APIs for combining futures, such as ``EventLoopFuture/and(_:)``.
+This potentially represents an issue, as two futures may not share the same isolation domain.
+As a result, we can only safely call these combining functions when the ``EventLoopFuture``
+values are `Sendable`.
+
+> Note: We can conceptually relax this constraint somewhat by offering equivalent
+    functions that can only safely be called when all the combined futures share the
+    same bound event loop: that is, when they are all within the same isolation domain.
+
+    This can be enforced with runtime isolation checks. If you have a need for these
+    functions, please reach out to the NIO team.
+
+## Interacting with Futures on the Event Loop
+
+In a number of contexts (such as in ``ChannelHandler``s), the programmer has static knowledge
+that they are within an isolation domain. That isolation domain may well be shared with the
+isolation domain of many futures and promises with which they interact. For example,
+futures that are provided from ``ChannelHandlerContext/write(_:promise:)`` will be bound to
+the event loop on which the ``ChannelHandler`` resides.
+
+In this context, the `sending` constraint is unnecessarily strict. The future callbacks are
+guaranteed to fire on the same isolation domain as the ``ChannelHandlerContext``: no risk
+of data race is present. However, Swift Concurrency cannot guarantee this at compile time,
+as the specific isolation domain is determined only at runtime.
+
+In these contexts, today users can make their callbacks safe using ``NIOLoopBound`` and
+``NIOLoopBoundBox``. These values can only be constructed on the event loop, and only allow
+access to their values on the same event loop. These constraints are enforced at runtime,
+so at compile time these are unconditionally `Sendable`.
+
+> Warning: ``NIOLoopBound`` and ``NIOLoopBoundBox`` replace compile-time isolation checks
+    with runtime ones. This makes it possible to introduce crashes in your code. Please
+    ensure that you are 100% confident that the isolation domains align. If you are not
+    sure that the ``EventLoopFuture`` you wish to attach a callback to is bound to your
+    ``EventLoop``, use ``EventLoopFuture/hop(to:)`` to move it to your isolation domain
+    before using these types.
+
+> Note: In a future NIO release we intend to improve the ergonomics of this common problem
+    by offering a related type that can only be created from an ``EventLoopFuture`` on a
+    given ``EventLoop``. This minimises the number of runtime checks, and will make it
+    easier and more pleasant to write this kind of code.
+
+## Interacting with Event Loops on the Event Loop
+
+As with Futures, there are occasionally times where it is necessary to schedule
+``EventLoop`` operations on the ``EventLoop`` where your code is currently executing.
+
+Much like with ``EventLoopFuture``, you can use ``NIOLoopBound`` and ``NIOLoopBoundBox``
+to make these callbacks safe.
+
+> Warning: ``NIOLoopBound`` and ``NIOLoopBoundBox`` replace compile-time isolation checks
+    with runtime ones. This makes it possible to introduce crashes in your code. Please
+    ensure that you are 100% confident that the isolation domains align. If you are not
+    sure that the ``EventLoopFuture`` you wish to attach a callback to is bound to your
+    ``EventLoop``, use ``EventLoopFuture/hop(to:)`` to move it to your isolation domain
+    before using these types.
+
+> Note: In a future NIO release we intend to improve the ergonomics of this common problem
+    by offering a related type that can only be created from an ``EventLoopFuture`` on a
+    given ``EventLoop``. This minimises the number of runtime checks, and will make it
+    easier and more pleasant to write this kind of code.

Sources/NIOCore/EventLoop+Deprecated.swift

@@ -23,9 +23,10 @@ extension EventLoop {
         self.makeFailedFuture(error)
     }
 
+    @preconcurrency
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func makeSucceededFuture<Success>(
+    public func makeSucceededFuture<Success: Sendable>(
         _ value: Success,
         file: StaticString = #fileID,
         line: UInt = #line