Skip to content

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Mar 10, 2023

This PR tracks an example of custom actor executor support, as well as updating our AsyncAwait example to clearly annotate (via precondition[Not]OnEventLoop) whether the code in question is executing on the event loop or not.

The initial conclusion here is that this demonstrates the strengths and limitations of custom actor executors for the NIO model. Actors running on an event loop executor clearly run on the event loop, such that the NIO fast-path successfully fires. However, free functions and non-actor types do not inherit this execution context, which limits the utility of our existing async sequences (which are not actors and cannot be EL bound).

@FranzBusch
Copy link
Member

Great PoC!

However, free functions and non-actor types do not inherit this execution context, which limits the utility of our existing async sequences (which are not actors and cannot be EL bound).

Can we try out marking our NIOAsyncSequenceProducer methods with @_inheritActorContext and having an example how this works with NIOAsyncChannel? In theory if the actor that is consuming the NIOAsyncSequenceProducer is using the NIO executor we should get a fast path right?

@John-Connolly
Copy link
Contributor

This is super interesting I'm eager to see what you come up with. It would be ideal if swift had a way to express that everything in this closure will get executed on a specific executor unless Task.detached is called or another task with a specified actor is started. I'm unsure how to model a Request -> Response scenario without that. The only solution i've come up with is to pass and isolated Actor as a parameter to every function I want to be run on a executor.

@ktoso ktoso self-requested a review March 13, 2023 01:15
@Lukasa Lukasa force-pushed the cb-custom-actor-executor branch from 358dc1a to ca0be2f Compare July 20, 2023 11:39
@Lukasa Lukasa force-pushed the cb-custom-actor-executor branch 2 times, most recently from 0b2211e to 6479d17 Compare July 27, 2023 12:23
@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Jul 27, 2023
@Lukasa Lukasa force-pushed the cb-custom-actor-executor branch from 6479d17 to 5ba3362 Compare July 27, 2023 12:27
@Lukasa Lukasa marked this pull request as ready for review July 27, 2023 12:27
Comment on lines +446 to +448
public var executor: any SerialExecutor {
NIODefaultSerialEventLoopExecutor(self)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we really should default to this or are better off with a fatalError. We are making sure our ELs implement this sensible but if somebody created their own EL and we use the default I am a bit concerned that users will just run into crashes for which we could provide a better fatalError message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to want to leave it. The current behaviour will work in the vast majority of cases, and the crashes will be correctness issues anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah probably it's okay since all our ELs do the right thing and there aren't many other ELs out there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okey to leave this IMHO 👍

@Lukasa
Copy link
Contributor Author

Lukasa commented Jul 27, 2023

Applied your nit.

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Too bad we couldn't put it onto EL right away but this is okey

//
//===----------------------------------------------------------------------===//

#if swift(>=5.9)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just circling back here. If we want there is a way to create a custom serial executor without warnings on 5.9 and blow like this

#if swift(<5.9)
final class MyExecutor: SerialExecutor {
    func enqueue(_ job: UnownedJob) {
        job._runSynchronously(on: self.asUnownedSerialExecutor())
    }

    func asUnownedSerialExecutor() -> UnownedSerialExecutor {
        UnownedSerialExecutor(ordinary: self)
    }
}
#else
@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
final class MyExecutor: SerialExecutor {
    @available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
    func enqueue(_ job: __owned ExecutorJob) {
      job.runSynchronously(on: asUnownedSerialExecutor())
    }

    func asUnownedSerialExecutor() -> UnownedSerialExecutor {
        UnownedSerialExecutor(ordinary: self)
    }
}
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Franz. It's a good suggestion but I'm inclined not to do it: using both _runSynchronously and __owned makes me really quite nervous about the stability of that code. @weissi are you happy with having this API on 5.9 only?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with having APIs that are available everywhere from a certain Swift version. But it should be #if compiler not #if swift

Lukasa added 2 commits July 31, 2023 13:39
Motivation

Swift Concurrency supports having actors opt in to executing
within a "custom executor". This requires being able to be strictly
serial, a constraint that NIO's EventLoops natively meet. It is
useful to have actors be able to express a "tie" to an EventLoop
in order to efficiently access objects that are tied to specific ELs.

Modifications

- Extend EventLoop to expose an executor property
- Provide a simple best-effort default using NIODefaultSerialEventLoopExecutor
- Expose NIOSerialEventLoopExecutor protocol that makes it easy
  for adopters to get a better implementation
- Crash if EmbeddedEventLoop is used
- Add tests

Result

Adopters can use ELs as serial executors
@Lukasa Lukasa force-pushed the cb-custom-actor-executor branch from 1423ad8 to 2478733 Compare July 31, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants