Merge pull request #2716 from ktoso/wip-isIsolated

[Concurrency] isIsolatingCurrentContext - Improved Custom SerialExecutor isolation checking

Author: DougGregor

proposals/NNNN-SerialExecutor-isIsolated.md

@@ -0,0 +1,196 @@
+# Improved Custom SerialExecutor isolation checking for Concurrency Runtime 
+
+* Proposal: [SE-NNNN](...)
+* Author: [Konrad 'ktoso' Malawski](https://github.com/ktoso)
+* Review Manager: TBD
+* Status:  Implemented
+  * https://github.com/swiftlang/swift/pull/79788
+  * https://github.com/swiftlang/swift/pull/79946
+
+* Pitch: [[Pitch][SerialExecutor] Improved Custom SerialExecutor isolation checking](https://forums.swift.org/t/pitch-serialexecutor-improved-custom-serialexecutor-isolation-checking/78237/)
+* Review: TODO
+
+## Introduction
+
+In [SE-0424: Custom isolation checking for SerialExecutor](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0424-custom-isolation-checking-for-serialexecutor.md) we introduced a way for custom executors implementing the `SerialExecutor` protocol to assert and assume the static isolation if the dynamic check succeeded. This proposal extends these capabilities, allowing custom executors to not only "check and crash if assumption was wrong", but also check and act on the result of the check.
+
+## Motivation
+
+The previously ([SE-0424](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0424-custom-isolation-checking-for-serialexecutor.md)) introduced family of `Actor/assertIsolated()`, `Actor/preconditionIsolated()` and `Actor/assumeIsolated(operation:)` all rely on the `SerialExecutor/checkIsolated()` API which was introduced in the same proposal.
+
+These APIs all follow the "*pass or crash*" pattern. Where the crash is caused in order to prevent an incorrect assumption about isolation resulting in unsafe concurrent access to some isolated state. This is frequently used by methods which are "known to be called" on some specific actor to recover the dynamic (known at runtime) isolation information, into its static equivalent and therefore safely access some isolated state, like in this example:
+
+```swift
+@MainActor 
+var counter: Int = num
+
+protocol P {
+  // Always called by the main actor, 
+  // yet protocol author forgot to annotate the method or protocol using @MainActor
+  func actuallyWeKnowForSureThisIsCalledFromTheMainActor()
+}
+
+struct Impl: P {
+  func actuallyWeKnowForSureThisIsCalledFromTheMainActor() {
+    MainActor.assumeIsolated { // we know this is safe here
+      counter += 1
+    }
+  }
+}
+
+@MainActor
+func call(p: some P) {
+  p.actuallyWeKnowForSureThisIsCalledFromTheMainActor()
+}
+```
+
+This works fine for many situations, however some libraries may need to be more careful when rolling out strict concurrency checks like these, and instead of crashing may want to choose to issue warnings before they adopt a mode that enforces correct use.
+
+Currently all APIs available are using the `checkIsolated()` API which must crash if called from a context not managed by the serial executor it is invoked on. This method is often implemented using `dispatchPrecondition()` when the serial executor is backed using `Dispatch` or custom `fatalError()` messages otherwise:
+
+```swift
+final class ExampleExecutor: SerialExecutor {
+  func checkIsolated() {
+    dispatchPrecondition(condition: .onQueue(self.queue))
+  }
+}
+```
+
+This approach is better than not being able to participate in the checks at all (i.e. this API allows for advanced thread/queue sharing between actor and non-actor code), but it has two severe limitations:
+
+- the crash **messages** offered by these `checkIsolated()`  crashes **are often sub-optimal** and confusing
+  - messages often don't include crucial information about which actor/executor the calling context was _actually_ executing on. Offering only "expected [...]" messages, leading to hard to debug crashes.
+- it is **impossible** for the Swift runtime to offer **isolation violation warnings**
+  - because the Swift runtime _must_ call into a custom executor to verify its isolation, the "pass or crash" method will crash, rather than inform the runtime that a violation occurred and we should warn about it.
+
+Today, it is not possible for the Swift runtime to issue _warnings_ if something is detected to be on not the expected executor, but somehow we'd still like to continue without crashing the application. 
+
+And for existing situations when `assumeIsolated()` is called and _should_ crash, by using this new API the Swift runtime will be able to provide _more informative_ messages, including all available context about the execution environment.
+
+## Proposed solution
+
+We propose to introduce a new `isIsolatingCurrentContext` protocol requirement to the `SerialExecutor` protocol:
+
+```swift
+protocol SerialExecutor {
+
+  /// May be called by the Swift runtime before `checkIsolated()`
+  /// in order to check for isolation violations at runtime, 
+  /// and potentially issue isolation warnings.
+  ///
+  /// [...]
+  func isIsolatingCurrentContext() -> Bool
+
+  // existing API, since SE-0424
+  @available(SwiftStdlib 6.0, *)
+  func checkIsolated() // must crash if run on a context not managed by this serial executor
+  
+  // ... 
+}
+
+extension SerialExecutor { 
+  /// Default implementation for backwards compatibility.
+  func isIsolatingCurrentContext() -> Bool { false }
+}
+```
+
+The Swift runtime is free to call the `isIsolated` function whenever it wants to verify if the current context is appropriately isolated by some serial executor. 
+
+In most cases implementing this new API is preferable to implementing `checkIsolated()`, as the Swift runtime is able to offer more detailed error messages when an isolation failure detected by a call to `isIsolatingCurrentContext()` is detected.
+
+## Detailed design
+
+The newly proposed `isIsolatingCurrentContext()` function participates in the previously established runtime isolation checking flow, and happens _before_ any calls to `checkIsolated()` are attempted. The following diagram explains the order of calls issued by the runtime to dynamically verify an isolation when e.g. `assumeIsolated()` is called:
+
+![diagram illustrating which method is called when](nnnn-is-isolated-flow.png)
+
+
+
+There are a lot of conditions here and availability of certain features also impacts this decision flow, so it is best to refer to the diagram for detailed analysis of every situation. However the most typical situation involves executing on a task, which has a potentially different executor than the `expected` one. In such situation the runtime will:
+
+- check for the existence of a "current" task,
+- (fast path) if a task is present: 
+  - compare the current task's serial executor it is isolated to (if any) with the expected serial executor,
+
+- :new: if **`isIsolatingCurrentContext`** **is available** on the `expected`  executor:
+  - invoke `isIsolatingCurrentContext` 
+  - ✅ if it returned true: pass the check.
+  - :x: if it returned false: fail the check.
+  - The runtime will **not** proceed to call `checkIsolated` after `isIsolated` is invoked!
+
+- if **`isIsolatingCurrentContext`** is **not available** on the expected executor, but **`checkIsolated`** **is available**:
+  - invoke `expected.checkIsolated` which will crash :x: or pass :white_check_mark: depending on its internal checking.
+- if neither `checkIsolated` or `isIsolatingCurrentContext` are available, 
+  - :x: crash with a best effort message.
+
+
+This proposal specifically adds the "if `isIsolatingCurrentContext` is available" branch into the existing logic for confirming the isolation expectation.
+
+If `isIsolatingCurrentContext` is available, effectively it replaces `checkIsolated` because it does offer a sub-par error message experience and is not able to offer a warning if Swift would be asked to check the isolation but not crash upon discovering a violation.
+
+### Detecting the `isIsolatingCurrentContext` checking mode
+
+The `isIsolatingCurrentContext` method effectively replaces the `checkIsolated` method, because it can answer the same question _if it is implemented_.
+
+Some runtimes may not be able to implement a the returning `isIsolatingCurrentContext`, and they are not required to implement the new protocol requirement.
+
+The general guidance about which method to implement is to implement `isIsolatingCurrentContext` whenever possible. This method can be used by the Swift runtime in "warning mode". When running a check in this mode, the `checkIsolated` method cannot and will not be used because it would cause an unexpected crash. A runtime may still want to implement the `checkIsolated` function if it truly is unable to return a true/false response to the isolation question, but can only assert on an illegal state. This function will not be used when the runtime does not expect a potential for a crash.
+
+The presence of a non-default implementation of the `isIsolatingCurrentContext` protocol witness is detected by the compiler and the runtime can detect this information in order to determine if the new function should be used for these checks. In other words, if there is an implementation of the requirement available _other than_ the default one provided in the concurrency library, the runtime will attempt to use this method _over_ the `checkIsolated` API. This allows for a smooth migration to the new API, and enables the use of this method in if the runtime would like issue a check that cannot cause a crash.
+
+### Compatibility strategy for custom SerialExecutor authors
+
+New executor implementations should prioritize implementing `isIsolatingCurrentContext` when available, using an appropriate `#if swift(>=...)` check to ensure compatibility. Otherwise, they should fall back to implementing the crashing version of this API: `checkIsolated()`.
+
+For authors of custom serial executors, adopting this feature is an incremental process and they can adopt it at their own pace, properly guarding the new feature with necessary availability guards. This feature requires a new version of the Swift concurrency runtime which is aware of this new mode and therefore able to call into the new checking function, therefore libraries should implement and adopt it, however it will only manifest itself when the code is used with a new enough concurrency runtime
+
+As a result, this change should cause little to no disruption to Swift concurrency users, while providing an improved error reporting experience if using executors which adopt this feature.
+
+## Source Compatibility
+
+This proposal is source compatible, a default implementation of the new protocol requirement is introduced along with it, allowing existing `SerialExecutors` to compile without changes.
+
+## Binary Compatibility
+
+This proposal is ABI additive, and does not cause any binary incompatibility risks.
+
+## Future directions
+
+### Expose `isIsolated` on (Distributed)Actor and MainActor?
+
+We are _not_ including new public API on Actor types because of concerns of this function being abused.
+
+If we determine that there are significant good use-cases for this method to be exposed, we might reconsider this position.
+
+## Alternatives considered
+
+### Somehow change `checkIsolated` to return a bool value
+
+This would be ideal, however also problematic since changing a protocol requirements signature would be ABI breaking.
+
+### Deprecate `checkIsolated`?
+
+In order to make adoption of this new mode less painful and not cause deprecation warnings to libraries which intend to support multiple versions of Swift, the `SerialExcecutor/checkIsolated` protocol requirement remains _not_ deprecated. It may eventually become deprecated in the future, but right now we have no plans of doing so.
+
+### Offer a tri-state return value rather than `Bool`
+
+We briefly considered offering a tri-state `enum DetectedSerialExecutorIsolation` as the return value of `isIsolatingCurrentContext`, however could not find many realistic use-cases for it.
+
+The return type could be defined as:
+
+```swift
+// not great name
+enum DetectedSerialExecutorIsolation {
+  case isolated // returned when isolated by this executor
+  case notIsolated // returned when definitely NOT isolated by this executor
+  case unknown // when the isIsolatingCurrentContext could not determine if the caller is isolated or not
+}
+```
+
+If we used the `.unknown` as default implementation of the new protocol requirement, this would allow for programatic detection if we called the default implementation, or an user provided implementation which could check a proper isolated/not-isolated state of the executing context.
+
+Technically there may exist new implementations which return the `.unknown` however it would have to be treated defensively as `.notIsolated` in any asserting APIs or other use-cases which rely on this check for runtime correctness. We are uncertain if introducing this tri-state is actually helpful in real situations and therefore the proposal currently proposes the use of a plain `Bool` value. 
+
+## Changelog
+
+- removed the manual need to signal to the runtime that the specific executor supports the new checking mode. It is now detected by the compiler and runtime, checking for the presence of a non-default implementation of the protocol requirement.