swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.02k stars 10.33k forks source link

assumeIsolated doesn't work for actors with Dispatch executors #74626

Open wadetregaskis opened 1 month ago

wadetregaskis commented 1 month ago

Description

assumeIsolated apparently doesn't understand custom executors, at all. So it lies, in essence, by crashing when in fact the code is executing isolated to the actor.

Reproduction

import Dispatch

actor Example {
    final class DispatchQueueExecutor: SerialExecutor {
        private let queue: DispatchQueue

        init(queue: DispatchQueue) {
            self.queue = queue
        }

        func enqueue(_ job: UnownedJob) {
            self.queue.async {
                job.runSynchronously(on: self.asUnownedSerialExecutor())
            }
        }

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

    private let executor: DispatchQueueExecutor

    init(queue: DispatchQueue) {
        self.executor = DispatchQueueExecutor(queue: queue)
    }

    func bridge() {
        print("Sadly, I never get my moment.")
    }

    nonisolated var unownedExecutor: UnownedSerialExecutor {
        executor.asUnownedSerialExecutor()
    }
}

let queue = DispatchQueue(label: "Example")
let example = Example(queue: queue)

queue.sync {
    example.assumeIsolated {
        $0.bridge()
    }
}

Expected behavior

Not crashing.

Environment

Swift 6 (swiftlang-6.0.0.3.300 clang-1600.0.20.10).

Additional information

No response

wadetregaskis commented 1 month ago

Note also that it makes no difference whether DispatchQueue or DispatchSerialQueue is used for the type, to whether it crashes (but it matters a lot for broader compatibility - DispatchSerialQueue was only added recently, in the 2023 Apple OS releases).

wadetregaskis commented 1 month ago

[The docs](https://developer.apple.com/documentation/swift/serialexecutor/checkisolated()-5c5my) claim there's a checkIsolated instance method available, but it doesn't appear to actually exist - the compiler barfs when I use the override keyword to try to implement it.

(tangentially, that documentation is very confusing anyway, since it says "Required" in big bold letters immediately followed by "Default implementation provided")

wadetregaskis commented 1 month ago

Also, more broadly, I'm not sure there is any way to actually implement checkIsolated myself, anyway. All the relevant Dispatch APIs, for checking if I'm currently on a desired queue, are private (or were removed long ago).

So I'm pretty befuddled, since I thought using a Dispatch queue for an actor's executor was the exemplar use-case for custom actor executors, yet it's clear that it doesn't currently work and it's not clear how it can ever work?!

pbk20191 commented 4 weeks ago

First of all, there was a detailed discussion from 0424-custom-isolation-checking-for-serialexecutor

Required Default implementation provided.

This is common documentation in swift protocols. Which means this property or method is required to implement, but the provider has a chance to provide default implementation. See Providing-Default-Implementations

[The docs](https://developer.apple.com/documentation/swift/serialexecutor/checkisolated()-5c5my) claim there's a checkIsolated instance method available, but it doesn't appear to actually exist - the compiler barfs when I use the override keyword to try to implement it.

checkIsolated should be implemented without using override keyword. And "Default implementation" of checkIsolated is simply a runtime fatalError.

Also, more broadly, I'm not sure there is any way to actually implement checkIsolated myself, anyway. All the relevant Dispatch APIs, for checking if I'm currently on a desired queue, are private (or were removed long ago).

So I'm pretty befuddled, since I thought using a Dispatch queue for an actor's executor was the exemplar use-case for custom actor executors, yet it's clear that it doesn't currently work and it's not clear how it can ever work?!

you can use dispatchpredicate to implement checkIsolated

wadetregaskis commented 4 weeks ago

checkIsolated should be implemented without using override keyword. And "Default implementation" of checkIsolated is simply a runtime fatalError.

It's never called, in any case. e.g. amending my earlier example code:

        private final class DispatchQueueExecutor: SerialExecutor {
            …

            func checkIsolated() {
                print("checkIsolated()")
            }
        }

The stacktrace is:

#0  0x0000000192dcd890 in _swift_runtime_on_report ()
#1  0x0000000192e8cb10 in _swift_stdlib_reportFatalErrorInFile ()
#2  0x0000000192a5ed94 in closure #1 in closure #1 in _assertionFailure(_:_:file:line:flags:) ()
#3  0x0000000192a5ded8 in _assertionFailure(_:_:file:line:flags:) ()
#4  0x000000010024f074 in Actor.assumeIsolated<τ_0_0>(_:file:line:) ()

you can use dispatchpredicate to implement checkIsolated

Ah, thank you! I had no idea that API existed.

wadetregaskis commented 4 weeks ago

Note that isSameExclusiveExecutionContext(other:) is also never called. It's not clear if it's expected to be in this context (assumeIsolated), though.

pbk20191 commented 4 weeks ago

It's never called, in any case. e.g. amending my earlier example code:

That looks weird. In current Swift 6 runtime, calling assumeIsolated outside of Task context, falls to here. I think you are compiling against swift 6 and running on swift 5 runtime maybe?

Note that isSameExclusiveExecutionContext(other:) is also never called. It's not clear if it's expected to be in this context (assumeIsolated), though.

This is expected behavior, isSameExclusiveExecutionContext is Only evaluated when current calling context is owned by a existing Task and have a SerialExecutor type. Since you're calling assumeIsolated outside of Task, Concurrency can not find any SerialExecutor to compare.

wadetregaskis commented 4 weeks ago

It's never called, in any case. e.g. amending my earlier example code:

That looks weird. In current Swift 6 runtime, calling assumeIsolated outside of Task context, falls to here. I think you are compiling against swift 6 and running on swift 5 runtime maybe?

…maybe? I definitely was compiling in Swift 5 mode previously, but even after adding .swiftLanguageVersion(.v6) to my package, the problem persists.

I'm not sure how to tell what version of the runtime I'm running against…?

hborla commented 1 week ago

@wadetregaskis Could you please let us know which OS you are running on? It could also be a back deployment issue. It's just the build version number / beta number of whatever device you ran the code on where you saw the crash. A complete crash log will also contain this information in the "OS Version" field in the header.

ktoso commented 6 days ago

I should explain the issue first perhaps, as there's some confusion what actually seems to not be working here.

First,

queue.sync {
    example.assumeIsolated {
        $0.bridge()
    }
}

this used to not work; but is the exact reason SE-0424 as proposed for Swift 6. The problem isn't about custom executors per se, but is about the fact that there is no executor at all to compare with here: queue.sync {; As far as Swift is concerned this is just some random thread running some synchronous code, and then you'd try to compare the example's executor with "no executor" so yeah it used to fail.

Now, this use-case is valid, so we provide checkIsolated to handle it -- the target executor can be asked by Swift "well, I've got no idea; but do you think this is actually you?" and checkIsolated on the expected executor. This should be implemented as dispatchPrecondition(condition: .onQueue(queue)) as the proposal shows.


Why isn't it getting invoked for you?

I suspect an old runtime, so it doesn't get invoked, as simple as that. Please update your OS and compiler.

This is on a recent macOS 15 which has checkIsolated in the library:

ktoso@dynames [19:47:53] [/tmp/MyExecutable] [main *]
-> % sudo xcode-select --switch /Applications/Xcode2.app

ktoso@dynames [19:47:53] [/tmp/MyExecutable] [main *]
-> % swift -version
swift-driver version: 1.111 Apple Swift version 6.0 (swiftlang-6.0.0.5.15 clang-1600.0.22.6)
Target: arm64-apple-macosx15.0

ktoso@dynames [19:48:01] [/tmp/MyExecutable] [main *]
-> % swift build -c release
Building for production...
[5/5] Linking MyExecutable
Build complete! (0.61s)

ktoso@dynames [19:48:09] [/tmp/MyExecutable] [main *]
-> % .build/release/MyExecutable
Checking DispatchQueueExecutor.checkIsolated()
Sadly, I never get my moment.
Nevermind... I did!

If you'd try to select an old Xcode it'd crash as you say:

-> % sudo xcode-select --switch /Applications/Xcode.app
swift runtime: backtrace-on-crash is not supported for privileged executables.
ktoso@dynames [19:49:28] [/tmp/MyExecutable] [main *]
-> % swift -version
swift-driver version: 1.87.1 Apple Swift version 5.9 (swiftlang-5.9.0.128.108 clang-1500.0.40.1)
Target: arm64-apple-macosx15.0
ktoso@dynames [19:49:30] [/tmp/MyExecutable] [main *]
-> % swift -version
swift-driver version: 1.87.1 Apple Swift version 5.9 (swiftlang-5.9.0.128.108 clang-1500.0.40.1)
Target: arm64-apple-macosx15.0
ktoso@dynames [19:49:32] [/tmp/MyExecutable] [main *]
-> % swift build -c release
Building for production...
remark: Incremental compilation has been disabled: it is not compatible with whole module optimization
[2/2] Linking MyExecutable
Build complete! (8.49s)
ktoso@dynames [19:50:44] [/tmp/MyExecutable] [main *]
-> % .build/release/MyExecutable
MyExecutable/main.swift:54: Fatal error: Incorrect actor executor assumption; Expected same executor as MyExecutable.Example.

💣 Program crashed: Signal 5: Backtracing from 0x1a4f412c4... failed

Long story short: update your OS and Xcode please, I believe this works as expected. The feature does not backdeploy.


diff --git a/Sources/main.swift b/Sources/main.swift
index e7b7bed..f4841e2 100644
--- a/Sources/main.swift
+++ b/Sources/main.swift
@@ -12,6 +12,7 @@ actor Example {

     func bridge() {
         print("Sadly, I never get my moment.")
+        print("Nevermind... I did!")
     }

     nonisolated var unownedExecutor: UnownedSerialExecutor {
@@ -37,7 +38,11 @@ final class DispatchQueueExecutor: SerialExecutor {
     public func asUnownedSerialExecutor() -> UnownedSerialExecutor {
         UnownedSerialExecutor(ordinary: self)
     }
-
+
+    public func checkIsolated() {
+        print("Checking \(Self.self).\(#function)")
+        dispatchPrecondition(condition: .onQueue(queue))
+    }
 }
wadetregaskis commented 5 days ago

Could you please let us know which OS you are running on?

macOS Sonoma 14.5 (23F79).

Please update your OS and compiler.

I'm already running the latest OS (see above). I can try a newer Xcode 16 beta, though it'll take a day or two to download (yay bay area internet).

It sounds like you're implying that this fix isn't included in any released OS versions, only in future ones? That'd be a real bummer - this was already my last-ditch workaround to try to support Swift 6 with this "mixed-mode" actor / GCD code. I have no choice about using GCD - that's forced by the Apple API in question (NWPathMonitor). The existing GCD-using code is correct, but Swift 6 doesn't understand that and issues erroneous compiler warnings & errors. 😔

Dropping support for all released OS versions is a non-starter, and will be for a couple of years at least.

ktoso commented 5 days ago

To support this exact pattern:

queue.sync { // there's no executor here
    example.assumeIsolated { // this needs SDK/runtime with checkIsolated

you'll need new OS (e.g. macOS 15), yeah AFAIK.