Fix EventLoopFuture and EventLoopPromise under strict concurrency checking

FranzBusch · FranzBusch · commit d95beafa532f · 2024-02-16T17:12:45.000Z
# 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()` 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.
diff --git a/Sources/NIOCore/AsyncAwaitSupport.swift b/Sources/NIOCore/AsyncAwaitSupport.swift
@@ -19,7 +19,7 @@ extension EventLoopFuture {
     /// function and want to get the result of this future.
     @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
     @inlinable
-    public func get() async throws -> Value {
+    public func get() async throws -> Value where Value: Sendable {
         return try await withUnsafeThrowingContinuation { (cont: UnsafeContinuation<UnsafeTransfer<Value>, Error>) in
             self.whenComplete { result in
                 switch result {
@@ -61,7 +61,9 @@ extension EventLoopPromise {
     @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
     @discardableResult
     @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()
@@ -334,7 +336,9 @@ struct AsyncSequenceFromIterator<AsyncIterator: AsyncIteratorProtocol>: AsyncSeq
 @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
 extension EventLoop {
     @inlinable
-    public func makeFutureWithTask<Return>(_ body: @Sendable @escaping () async throws -> Return) -> EventLoopFuture<Return> {
+    public func makeFutureWithTask<Return: Sendable>(
+        _ body: @Sendable @escaping () async throws -> Return
+    ) -> EventLoopFuture<Return> {
         let promise = self.makePromise(of: Return.self)
         promise.completeWithTask(body)
         return promise.futureResult
diff --git a/Sources/NIOCore/DispatchQueue+WithFuture.swift b/Sources/NIOCore/DispatchQueue+WithFuture.swift
@@ -28,9 +28,9 @@ 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>(
+    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)
 
diff --git a/Sources/NIOCore/EventLoop.swift b/Sources/NIOCore/EventLoop.swift
@@ -712,13 +712,7 @@ extension EventLoop {
     /// - returns: An `EventLoopFuture` containing the result of `task`'s execution.
     @inlinable
     @preconcurrency
-    public func submit<T>(_ task: @escaping @Sendable () throws -> T) -> EventLoopFuture<T> {
-        _submit(task)
-    }
-    @usableFromInline typealias SubmitCallback<T> = @Sendable () throws -> T
-
-    @inlinable
-    func _submit<T>(_ task: @escaping SubmitCallback<T>) -> EventLoopFuture<T> {
+    public func submit<T: Sendable>(_ task: @escaping @Sendable () throws -> T) -> EventLoopFuture<T> { // TODO: This should take a closure that returns fresh
         let promise: EventLoopPromise<T> = makePromise(file: #fileID, line: #line)
 
         self.execute {
@@ -742,18 +736,15 @@ extension EventLoop {
     /// - returns: An `EventLoopFuture` identical to the `EventLoopFuture` returned from `task`.
     @inlinable
     @preconcurrency
-    public func flatSubmit<T>(_ task: @escaping @Sendable () -> EventLoopFuture<T>) -> EventLoopFuture<T> {
-        self._flatSubmit(task)
-    }
-    @usableFromInline typealias FlatSubmitCallback<T> = @Sendable () -> EventLoopFuture<T>
-
-    @inlinable
-    func _flatSubmit<T>(_ task: @escaping FlatSubmitCallback<T>) -> EventLoopFuture<T> {
+    public func flatSubmit<T: Sendable>(_ task: @escaping @Sendable () -> EventLoopFuture<T>) -> EventLoopFuture<T> { // TODO: This should take a closure that returns fresh
         self.submit(task).flatMap { $0 }
     }
 
     /// Schedule a `task` that is executed by this `EventLoop` at the given time.
     ///
+    /// - Note: The `T` must be `Sendable` since the isolation domains of the event loop future returned from `task` and
+    /// this event loop might differ.
+    ///
     /// - parameters:
     ///     - task: The asynchronous task to run. As with everything that runs on the `EventLoop`, it must not block.
     /// - returns: A `Scheduled` object which may be used to cancel the task if it has not yet run, or to wait
@@ -763,23 +754,11 @@ extension EventLoop {
     @discardableResult
     @inlinable
     @preconcurrency
-    public func flatScheduleTask<T>(
+    public func flatScheduleTask<T: Sendable>(
         deadline: NIODeadline,
         file: StaticString = #fileID,
         line: UInt = #line,
         _ task: @escaping @Sendable () throws -> EventLoopFuture<T>
-    ) -> Scheduled<T> {
-        self._flatScheduleTask(deadline: deadline, file: file, line: line, task)
-    }
-    @usableFromInline typealias FlatScheduleTaskDeadlineCallback<T> = () throws -> EventLoopFuture<T>
-
-    @discardableResult
-    @inlinable
-    func _flatScheduleTask<T>(
-        deadline: NIODeadline,
-        file: StaticString,
-        line: UInt,
-        _ task: @escaping FlatScheduleTaskDelayCallback<T>
     ) -> Scheduled<T> {
         let promise: EventLoopPromise<T> = self.makePromise(file: file, line: line)
         let scheduled = self.scheduleTask(deadline: deadline, task)
@@ -790,6 +769,9 @@ extension EventLoop {
 
     /// Schedule a `task` that is executed by this `EventLoop` after the given amount of time.
     ///
+    /// - Note: The `T` must be `Sendable` since the isolation domains of the event loop future returned from `task` and
+    /// this event loop might differ.
+    ///
     /// - parameters:
     ///     - task: The asynchronous task to run. As everything that runs on the `EventLoop`, it must not block.
     /// - returns: A `Scheduled` object which may be used to cancel the task if it has not yet run, or to wait
@@ -799,23 +781,11 @@ extension EventLoop {
     @discardableResult
     @inlinable
     @preconcurrency
-    public func flatScheduleTask<T>(
+    public func flatScheduleTask<T: Sendable>(
         in delay: TimeAmount,
         file: StaticString = #fileID,
         line: UInt = #line,
         _ task: @escaping @Sendable () throws -> EventLoopFuture<T>
-    ) -> Scheduled<T> {
-        self._flatScheduleTask(in: delay, file: file, line: line, task)
-    }
-    
-    @usableFromInline typealias FlatScheduleTaskDelayCallback<T> = @Sendable () throws -> EventLoopFuture<T>
-
-    @inlinable
-    func _flatScheduleTask<T>(
-        in delay: TimeAmount,
-        file: StaticString,
-        line: UInt,
-        _ task: @escaping FlatScheduleTaskDelayCallback<T>
     ) -> Scheduled<T> {
         let promise: EventLoopPromise<T> = self.makePromise(file: file, line: line)
         let scheduled = self.scheduleTask(in: delay, task)
@@ -951,7 +921,7 @@ extension EventLoop {
         notifying promise: EventLoopPromise<Void>?,
         _ task: @escaping ScheduleRepeatedTaskCallback
     ) -> RepeatedTask {
-        let futureTask: (RepeatedTask) -> EventLoopFuture<Void> = { repeatedTask in
+        let futureTask: @Sendable (RepeatedTask) -> EventLoopFuture<Void> = { repeatedTask in
             do {
                 try task(repeatedTask)
                 return self.makeSucceededFuture(())
diff --git a/Sources/NIOCore/EventLoopFuture+Deprecated.swift b/Sources/NIOCore/EventLoopFuture+Deprecated.swift
@@ -15,63 +15,91 @@
 extension EventLoopFuture {
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func flatMap<NewValue>(file: StaticString = #fileID, line: UInt = #line, _ callback: @escaping (Value) -> EventLoopFuture<NewValue>) -> EventLoopFuture<NewValue> {
+    public func flatMap<NewValue: Sendable>(
+        file: StaticString = #fileID,
+        line: UInt = #line,
+        _ callback: @escaping @Sendable (Value) -> EventLoopFuture<NewValue>
+    ) -> EventLoopFuture<NewValue> {
         return self.flatMap(callback)
     }
 
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func flatMapThrowing<NewValue>(file: StaticString = #fileID,
-                                line: UInt = #line,
-                                _ callback: @escaping (Value) throws -> NewValue) -> EventLoopFuture<NewValue> {
+    public func flatMapThrowing<NewValue: Sendable>(
+        file: StaticString = #fileID,
+        line: UInt = #line,
+        _ callback: @escaping @Sendable (Value) throws -> NewValue
+    ) -> EventLoopFuture<NewValue> {
         return self.flatMapThrowing(callback)
     }
 
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func flatMapErrorThrowing(file: StaticString = #fileID, line: UInt = #line, _ callback: @escaping (Error) throws -> Value) -> EventLoopFuture<Value> {
+    public func flatMapErrorThrowing(
+        file: StaticString = #fileID,
+        line: UInt = #line,
+        _ callback: @escaping @Sendable (Error) throws -> Value
+    ) -> EventLoopFuture<Value> {
         return self.flatMapErrorThrowing(callback)
     }
 
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func map<NewValue>(file: StaticString = #fileID, line: UInt = #line, _ callback: @escaping (Value) -> (NewValue)) -> EventLoopFuture<NewValue> {
+    public func map<NewValue>(
+        file: StaticString = #fileID,
+        line: UInt = #line,
+        _ callback: @escaping @Sendable (Value) -> (NewValue)
+    ) -> EventLoopFuture<NewValue> {
         return self.map(callback)
     }
 
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func flatMapError(file: StaticString = #fileID, line: UInt = #line, _ callback: @escaping (Error) -> EventLoopFuture<Value>) -> EventLoopFuture<Value> {
+    public func flatMapError(
+        file: StaticString = #fileID,
+        line: UInt = #line,
+        _ callback: @escaping @Sendable (Error) -> EventLoopFuture<Value>
+    ) -> EventLoopFuture<Value> where Value: Sendable {
         return self.flatMapError(callback)
     }
 
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func flatMapResult<NewValue, SomeError: Error>(file: StaticString = #fileID,
-                                                          line: UInt = #line,
-                                                          _ body: @escaping (Value) -> Result<NewValue, SomeError>) -> EventLoopFuture<NewValue> {
+    public func flatMapResult<NewValue, SomeError: Error>(
+        file: StaticString = #fileID,
+        line: UInt = #line,
+        _ body: @escaping @Sendable (Value) -> Result<NewValue, SomeError>
+    ) -> EventLoopFuture<NewValue> {
         return self.flatMapResult(body)
     }
 
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func recover(file: StaticString = #fileID, line: UInt = #line, _ callback: @escaping (Error) -> Value) -> EventLoopFuture<Value> {
+    public func recover(
+        file: StaticString = #fileID,
+        line: UInt = #line,
+        _ callback: @escaping @Sendable (Error) -> Value
+    ) -> EventLoopFuture<Value> {
         return self.recover(callback)
     }
 
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func and<OtherValue>(_ other: EventLoopFuture<OtherValue>,
-                                file: StaticString = #fileID,
-                                line: UInt = #line) -> EventLoopFuture<(Value, OtherValue)> {
+    public func and<OtherValue: Sendable>(
+        _ other: EventLoopFuture<OtherValue>,
+        file: StaticString = #fileID,
+        line: UInt = #line
+    ) -> EventLoopFuture<(Value, OtherValue)> {
         return self.and(other)
     }
 
     @inlinable
     @available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
-    public func and<OtherValue>(value: OtherValue,
-                                file: StaticString = #fileID,
-                                line: UInt = #line) -> EventLoopFuture<(Value, OtherValue)> {
+    public func and<OtherValue: Sendable>(
+        value: OtherValue,
+        file: StaticString = #fileID,
+        line: UInt = #line
+    ) -> EventLoopFuture<(Value, OtherValue)> {
         return self.and(value: value)
     }
 }
diff --git a/Sources/NIOCore/EventLoopFuture+WithEventLoop.swift b/Sources/NIOCore/EventLoopFuture+WithEventLoop.swift
@@ -41,7 +41,9 @@ extension EventLoopFuture {
     /// - returns: A future that will receive the eventual value.
     @inlinable
     @preconcurrency
-    public func flatMapWithEventLoop<NewValue>(_ callback: @escaping @Sendable (Value, EventLoop) -> EventLoopFuture<NewValue>) -> EventLoopFuture<NewValue> {
+    public func flatMapWithEventLoop<NewValue: Sendable>(
+        _ callback: @escaping @Sendable (Value, EventLoop) -> EventLoopFuture<NewValue>
+    ) -> EventLoopFuture<NewValue> {
         let next = EventLoopPromise<NewValue>.makeUnleakablePromise(eventLoop: self.eventLoop)
         self._whenComplete { [eventLoop = self.eventLoop] in
             switch self._value! {
@@ -75,7 +77,9 @@ extension EventLoopFuture {
     /// - returns: A future that will receive the recovered value.
     @inlinable
     @preconcurrency
-    public func flatMapErrorWithEventLoop(_ callback: @escaping @Sendable (Error, EventLoop) -> EventLoopFuture<Value>) -> EventLoopFuture<Value> {
+    public func flatMapErrorWithEventLoop(
+        _ callback: @escaping @Sendable (Error, EventLoop) -> EventLoopFuture<Value>
+    ) -> EventLoopFuture<Value> where Value: Sendable {
         let next = EventLoopPromise<Value>.makeUnleakablePromise(eventLoop: self.eventLoop)
         self._whenComplete { [eventLoop = self.eventLoop] in
             switch self._value! {
@@ -114,10 +118,11 @@ extension EventLoopFuture {
     /// - returns: A new `EventLoopFuture` with the folded value whose callbacks run on `self.eventLoop`.
     @inlinable
     @preconcurrency
-    public func foldWithEventLoop<OtherValue>(
+    public func foldWithEventLoop<OtherValue: Sendable>(
         _ futures: [EventLoopFuture<OtherValue>],
         with combiningFunction: @escaping @Sendable (Value, OtherValue, EventLoop) -> EventLoopFuture<Value>
-    ) -> EventLoopFuture<Value> {
+    ) -> EventLoopFuture<Value> where Value: Sendable {
+        @Sendable
         func fold0(eventLoop: EventLoop) -> EventLoopFuture<Value> {
             let body = futures.reduce(self) { (f1: EventLoopFuture<Value>, f2: EventLoopFuture<OtherValue>) -> EventLoopFuture<Value> in
                 let newFuture = f1.and(f2).flatMap { (args: (Value, OtherValue)) -> EventLoopFuture<Value> in
diff --git a/Sources/NIOCore/EventLoopFuture.swift b/Sources/NIOCore/EventLoopFuture.swift
diff --git a/Tests/NIOPosixTests/EventLoopFutureTest.swift b/Tests/NIOPosixTests/EventLoopFutureTest.swift
