SE-0424: Custom isolation checking for `SerialExecutor`

John_McCall · 2024-02-22T06:40:15.324Z

Hi, Swift community.

The review of SE-0424: Custom isolation checking for SerialExecutor begins now and runs through March 4th, 2024.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager. When emailing the review manager directly, please put "[SE-0424]" at the start of the subject line.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/main/process.md

Thank you,

John McCall
Review Manager

ktoso · 2024-02-22T09:39:25.730Z

We're currently building a toolchain so that people could try this out in action. I'll post it here once the build completes.

Toolchains:

Ubuntu 20.04 (x86_64)
macOS - pending

The implementation in the preview toolchain coming up is not compatible with -enable-actor-data-race-checks when configured to "warn" mode, and it will crash even if asked to just log warnings. We are working on addressing this limitation soon.

I would also like to point out the relationship of this proposal and the ongoing review of dynamic enforcement: SE-0423: Dynamic actor isolation enforcement from non-strict-concurrency contexts

The implicitly inserted checks discussed in that proposal, would also benefit from this improved checking semantics. This might be most interesting for the MainActor, where main actor functions are called from "raw Main queue", as discussed in this proposal.

mickeyl · 2024-02-22T12:04:36.534Z

As this is a straightforward enhancement of the custom actors protocol (almost a fix, if I may), I welcome this.

John_McCall · 2024-02-23T01:32:45.782Z

cbarrett · 2024-02-23T08:36:42.418Z

I'm in favor of this proposal. One request for clarification: why does checkIsolation() require its implementation to crash? Naively, one could imagine a function which returns false or throws instead of crashing if the last-effort isolation check fails. I assume there are good reasons not to do either of those things, so it would be nice to document those reasons in the "Alternatives Considered" section.

ktoso · 2024-02-23T08:47:08.306Z

cbarrett:

Naively, one could imagine a function which returns false or throws instead of crashing if the last-effort isolation check fails.

Yeah that is a good question and I'll add some discussion about it to the proposal for future readers.

So... the runtime currently IS implemented in terms if an "is on...?" API, and... we're in trouble because of that.

Dispatch currently cannot, or at least "does not want to offer API to answer such questions". This decision is informed by prior APIs existing that offered that and being abused a lot, I'm told. So, as Dispatch is an important underlying executor, we're somewhat forced into the "crash if assumption is wrong" model which Dispatch is comfortable offering: as the dispatchPrecondition API.

This is actually unfortunate, and makes backdeployment of this feature very rather difficult. So long story short, this shape of API is forced by API limitations of existing runtimes underlying Swift Concurrency -- we can't implement a boolean returning API, unless Dispatch were to expose such API, and in discussions it was explained that exposing such API is not something the team was comfortable with as it would potentially be abused left and right.

The problem sadly cannot be solved with some form of SPI either -- Dispatch is absolutely able to implement the true/false returning isIsolated if we wanted to use an SPI to do so. However, since isIsolated is a public API that exactly does the operation Dispatch do not want to expose... it is as-if Dispatch suddenly exposed this operation

It is a fair concern about making an API "slightly weird" because Dispatch's API preferences though. It is also true though that all other executors including NIO and other imaginary ones if they could have implemented the true/false one, they can also implement this shape.

Another downside to discuss is worse error reporting... The Swift runtime causing the crash could potentially form better error messages than the checkIsolated implementation might be able to... We know "there was no current executor", or we could look at the metadata of executors found and print them etc. This is problematic with the checkIsolated. We still be able improve this situation by offering an message: String parameter...?

hborla · 2024-02-24T19:20:18.033Z

John_McCall:

What is your evaluation of the proposal?

This proposal fills an important expressivity gap in the custom actor executor feature. A major use case for custom actor executors is bridging between other concurrency systems with actor-like serialized access to mutable state to enable writing new code using actors while interoperating with code using the old system. This feature is critical for using assumeIsolated when entering the actor through the old mechanism where actor isolation is not guaranteed statically.

I also believe the solution is straightforward - it adds a new requirement to SerialExecutor, which custom actor executors must already conform to, and protocol requirements are the standard way to provide customization points like this. This solution also paves to way to subsume the bespoke check for MainActor, further indicating the need for a general solution.

I have two specific questions:

Should the default implementation of checkIsolation() be deprecated so that when recompiled, SerialExecutor conformances get a warning prompting them to provide an explicit implementation? I'm fixing the compiler bug where the witness checker fails to diagnose this case now. If we think it'll be common to implement this requirement, I think it's worth a warning.
Are there situations where we expect the executor equality comparison to fail, but the checkIsolated() call to succeed? Or does is that situation indicative of a case where a custom executor should have provided a complex equality implementation but didn't?

John_McCall:

Is the problem being addressed significant enough to warrant a change to Swift?

Yes, the issue I described above prevents any access to actor isolated state when entering the actor through a bridged concurrency system. This prevents custom actor executors from being used to aid incremental migration over to actors from other concurrency systems, forcing programmers to choose between sticking with the old system or undertaking a major refactoring to completely rewrite the code to use actors.

John_McCall:

Does this proposal fit well with the feel and direction of Swift?

Yes, incremental migration to actors and other native concurrency primitives is a critical pillar of the Swift 6 migration.

John_McCall:

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I don't know of other languages that have facilitated a transition from existing concurrency systems to a new one with static data race safety, so I can't compare this feature to others.

John_McCall:

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I ran into this problem myself when attempting to implement an actor backed by a DispatchSerialQueue executor in existing projects that use delegate-callback-queue-based APIs, participated in discussions to understand the problem and the potential solution space, and spent about an hour going through the written proposal.

ktoso · 2024-02-26T06:44:54.043Z

hborla:

Should the default implementation of checkIsolation() be deprecated so that when recompiled, SerialExecutor conformances get a warning prompting them to provide an explicit implementation? I'm fixing the compiler bug where the witness checker fails to diagnose this case now. If we think it'll be common to implement this requirement, I think it's worth a warning.

I'd lean on the side of no. The default implementation is a pretty good default and not something that necessarily "must" be overriden by implementations.

I do think though that we need to expand our documentation to explain this ability to conformers of the protocol -- both in API docs and language guide that should get more examples maybe etc.

I'm also iffy about adding such deprecation warnings after the last time we tried to do such thing with serial executors and the enqueue overload and it caused months of pain and in the end we removed the warning as libraries supporting multiple versions had a rough time adopting the protocol's new methods... In this case I think it'd be easier to just provide an impl if Swift 6+ but I don't think it's worth it to be honest.

hborla:

Are there situations where we expect the executor equality comparison to fail, but the checkIsolated() call to succeed? Or does is that situation indicative of a case where a custom executor should have provided a complex equality implementation but didn't?

Let's use the dispatch example, since it is one of the "lots of different meanings of equality" one.

Dispatch, if/when it'd implement the complex equality checking logic we have provided for it, then it is able to do the "deep" "complex" (check target queues etc), checks indeed in the executor comparison.

When Dispatch implements the complex comparison, and there is a "current" executor, indeed that branch should always be able to handle the comparison.

When there is no "current" to compare against, we'd enter the newly proposed API.

This actually brings up an interesting point... Dispatch was reluctant to offer a "compare" Bool returning API when we discussed this, but we already have such "compare" style API we wanted to implement in form of the isSameExclusiveExecutionContext, so perhaps we should discuss again if we truly cannot do a bool returning API here as well?

As we introduced a bool returning API in 5.9 that we wanted to implement a while back:

@available(SwiftStdlib 5.9, *)
  func isSameExclusiveExecutionContext(other: Self) -> Bool

(!)

I'll have to loop back to this shortly.

Nickolas_Pohilets · 2024-02-26T12:36:59.842Z

I would like to have a Bool returning API to support isolated deinit. Implementation of the isolated deinit uses swift_task_isCurrentExecutor() to choose between fast path (no hopping) and slow path (hopping).

I wonder what are other examples of SerialExecutor implementations, aside from the Dispatch-based. Would they benefit from having var isIsolated: Bool? in the SerialExecutor protocol, even if Dispatch-based implementation just returns nil?

But even for Dispatch I see an opportunity. If code passes expected.checkIsolated() check, then we could set Task.current.executor = expected for the duration of the block passed to assumeIsolated(). Then inside the block calls to swift_task_isCurrentExecutor(expected) would return true, enabling fast path of the isolated deinit.

AlexanderM · 2024-03-06T04:09:28.514Z

The API contract stood out to me on first reading, too (even before seeing the posts here), and I had the same thoughts about how the concurrency runtime is likely in a better position to provide good error messages than the “Swift userspace” executor code.

ktoso:

This is actually unfortunate, and makes backdeployment of this feature very rather difficult. So long story short, this shape of API is forced by API limitations of existing runtimes underlying Swift Concurrency -- we can't implement a boolean returning API, unless Dispatch were to expose such API

Is there potential here for a middle ground solution where the protocol requires a Bool-returning function (or property?) which is allowed (but not obligated) to fatalError?

This would accommodate Dispatch, while leaving a better API design for the language going forward.

John_McCall · 2024-03-06T23:37:22.713Z

SE-0424 has been accepted. Thank you all for participating in this review.

John McCall
Review Manager

John_McCall · 2024-03-06T23:37:27.026Z

ktoso · 2024-03-06T23:45:04.331Z

Thank you for the feedback everyone. Thanks for the review summary @John_McCall ! It indeed sounds like a good plan that if we ever need other APIs we can add them.