Add isolated views to EventLoop, Promise, and Future #2969

Lukasa · 2024-11-07T16:29:40Z

Motivation:

Users writing NIO code in a strict concurrency world often need to interact with futures, promises, and event loops. The main interface to these has strict sendability requirements, as it is possible the user is doing so from outside the EventLoop that provides the isolation domain for these types.

However, in many cases the user knows that they are on the isolation domain in question. In that case, they need more capabilities. While they can achieve their goals with NIOLoopBound today, it'd be nice if they had a better option.

Modifications:

Make EventLoop.Isolated public.
Make EventLoopFuture.Isolated public.
Make EventLoopPromise.Isolated public.
Make all their relevant methods public.
Move the runtime isolation check from the point of use to the point of construction.
Make the types non-Sendable to ensure that isolation check is sufficient.
Add unsafeUnchecked options to create these types when performance matters and correctness is clear.
Add tests for their behaviour.
Update the documentation.

Result:

Writing safe code with promises, futures, and event loops is easier.

Motivation: Users writing NIO code in a strict concurrency world often need to interact with futures, promises, and event loops. The main interface to these has strict sendability requirements, as it is possible the user is doing so from outside the EventLoop that provides the isolation domain for these types. However, in many cases the user knows that they are on the isolation domain in question. In that case, they need more capabilities. While they can achieve their goals with NIOLoopBound today, it'd be nice if they had a better option. Modifications: - Make EventLoop.Isolated public. - Make EventLoopFuture.Isolated public. - Make EventLoopPromise.Isolated public. - Make all their relevant methods public. - Move the runtime isolation check from the point of use to the point of construction. - Make the types non-Sendable to ensure that isolation check is sufficient. - Add unsafeUnchecked options to create these types when performance matters and correctness is clear. - Add tests for their behaviour. - Update the documentation. Result: Writing safe code with promises, futures, and event loops is easier.

glbrntt

This looks great, we're missing a NIO prefix though.

glbrntt · 2024-11-08T09:08:26Z

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

+///
+/// Using this type relaxes the need to have the closures for ``EventLoop/execute(_:)``,
+/// ``EventLoop/submit(_:)``, and ``EventLoop/scheduleTask(in:_:)`` to be `@Sendable`.
+public struct IsolatedEventLoop {


We need a NIO prefix here

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

glbrntt · 2024-11-08T09:17:19Z

Tests/NIOPosixTests/EventLoopFutureIsolatedTests.swift

+        let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
+        defer {
+            try! group.syncShutdownGracefully()
+        }


Not that it matters much, but should we prefer singletons in tests as they save a little boilerplate.

glbrntt · 2024-11-08T09:20:25Z

Tests/NIOPosixTests/EventLoopFutureIsolatedTests.swift

+
+            // This block is the main happy path.
+            let newFuture = future.flatMap { result in
+                XCTAssertTrue(originalValue === result)


FWIW: XCTAssertIdentical exists for this

glbrntt

LGTM, thanks Cory!

FranzBusch · 2024-11-18T08:19:25Z

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

    @inlinable
-    func execute(_ task: @escaping () -> Void) {
-        self._wrapped.assertInEventLoop()


Meta question: I understand why you removed those but should we keep them in in-case somebody really mishandles this type? In release builds these all should go away anyways.

FranzBusch · 2024-11-18T08:21:09Z

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

+///
+/// This type is explicitly not `Sendable`. It may only be constructed on an event loop,
+/// using ``EventLoop/assumeIsolated()``, and may not subsequently be passed to other isolation
+/// domains.


I think other isolation domains is actually too weak of a statement. This type must not be held inside any asynchronous context.

I think that's not appropriate either: non-asynchronous contexts are themselves dangerous. As task executors aren't supported today, I think the statement as written is correct. I would now reject any plan to add task executors until these issues are resolved, so I'm inclined to keep the text.

What non-asynchronous context are you referring to? I consider inside an actor an asynchronous context since you need to be inside a Task.

I agree, but I think that framing is going to be prone to misunderstanding, I think: people will see the noasync directive and believe that the word async signals the issue, which it doesn't.

FranzBusch · 2024-11-18T08:21:24Z

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

@@ -119,26 +130,54 @@ struct IsolatedEventLoop {

    /// Returns the wrapped event loop.
    @inlinable
-    func nonisolated() -> any EventLoop {
+    public func nonisolated() -> any EventLoop {


Can we make this method also noasync

I don't think we need to: this is the only method that is always safe to call on this type.

I understand your argument my reason is that if a user ever ends up holding this type in an async method I really want them to see an error. Even if that place is not where the error happened but they should question how they got the type there in the first place.

FranzBusch · 2024-11-18T08:21:35Z

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

+    /// isolation check in release builds. It retains it in debug mode to
+    /// ensure correctness.
+    @inlinable
+    public func assumeIsolatedUnsafeUnchecked() -> NIOIsolatedEventLoop {


Same here. Can we make this noasync

I deliberately didn't do that. This is already marked unsafe-unchecked. I have therefore left it for the possible use-case that someone is actually trying to do the thing we're trying to prevent them doing, and is willing to take on the risk of doing it correctly themselves.

Okay fair enough. Can we add that to the documentation then.

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

FranzBusch · 2024-11-18T08:25:10Z

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

+    /// versions for the functions that do require `Sendable` types. If you have an
+    /// ``EventLoopPromise/Isolated`` but need a regular ``EventLoopPromise``, use
+    /// ``EventLoopPromise/Isolated/nonisolated()`` to unwrap the value.
+    public struct Isolated {


I think all methods in this type or in general in all those new types should be marked noasync. None of them should ever be held in an async context.

FranzBusch · 2024-11-18T08:25:21Z

Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift

+    /// omits the runtime check in release builds. This improves performance, but
+    /// should only be used sparingly.
+    @inlinable
+    public func assumeIsolatedUnsafeUnchecked() -> Isolated {


Suggested change

public func assumeIsolatedUnsafeUnchecked() -> Isolated {

@available(*, noasync)

public func assumeIsolatedUnsafeUnchecked() -> Isolated {

Same note here, I want to leave the unsafe unchecked methods without the annotation.

Lukasa added the 🆕 semver/minor Adds new public API. label Nov 7, 2024

glbrntt requested changes Nov 8, 2024

View reviewed changes

Lukasa added 7 commits November 12, 2024 15:01

Namespace

66090d5

Merge remote-tracking branch 'origin/main' into cb-isolated

53e64de

Singletons

2d2c65e

XCTAssertIdentical

fa0672d

Format fix

e0fd516

Clean up documentation

28a02b9

Missed the one documentation one

ccff9c8

glbrntt approved these changes Nov 13, 2024

View reviewed changes

Lukasa added 2 commits November 17, 2024 16:54

Merge remote-tracking branch 'origin/main' into cb-isolated

8a67fe3

Unavailable from async

4714b90

FranzBusch reviewed Nov 18, 2024

View reviewed changes

glbrntt approved these changes Nov 18, 2024

View reviewed changes

Docs clarification

d3dc7b9

FranzBusch approved these changes Nov 18, 2024

View reviewed changes

Lukasa added 2 commits December 16, 2024 13:51

Merge branch 'main' into cb-isolated

eaf85fd

Merge branch 'main' into cb-isolated

4edbd6b

Lukasa merged commit 1e4fde1 into apple:main Dec 16, 2024
33 of 35 checks passed

Lukasa deleted the cb-isolated branch December 16, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isolated views to EventLoop, Promise, and Future #2969

Add isolated views to EventLoop, Promise, and Future #2969

Lukasa commented Nov 7, 2024

glbrntt left a comment

glbrntt Nov 8, 2024

glbrntt Nov 8, 2024

glbrntt Nov 8, 2024

glbrntt left a comment

FranzBusch Nov 18, 2024

FranzBusch Nov 18, 2024

Lukasa Nov 18, 2024

FranzBusch Nov 18, 2024

Lukasa Nov 18, 2024

FranzBusch Nov 18, 2024

Lukasa Nov 18, 2024

FranzBusch Nov 18, 2024

FranzBusch Nov 18, 2024

Lukasa Nov 18, 2024

FranzBusch Nov 18, 2024

FranzBusch Nov 18, 2024

FranzBusch Nov 18, 2024

Lukasa Nov 18, 2024

	public func assumeIsolatedUnsafeUnchecked() -> Isolated {
	@available(*, noasync)
	public func assumeIsolatedUnsafeUnchecked() -> Isolated {

Add isolated views to EventLoop, Promise, and Future #2969

Add isolated views to EventLoop, Promise, and Future #2969

Conversation

Lukasa commented Nov 7, 2024

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment