swift icon indicating copy to clipboard operation
swift copied to clipboard

Incorrect actor isolation assumption across Swift 5/6 module boundary leads to `dispatch_assert_queue` crashes

Open NachoSoto opened this issue 1 year ago • 29 comments

Reproduction

Consider the following code, split in 2 modules:

// Module 1, compiled with Swift 5 and no strict concurrency checking:
public class A {
  public init() {}

  public func f(_ value: @escaping () -> Void) {
    DispatchQueue.global().async { value() }
  }
}

// Module 2, compiled with Swift 6:
import Module1

@MainActor
func g() {
  A().f {
    // This is assumed to be @MainActor isolated by default, leading to `dispatch_assert_queue` failed assertion
  }
}

Expected behavior

The code compiles with no warnings (because the closure passed to f is valid based on the definition), but it crashes at runtime due to it being inferred as @MainActor, and executing outside of it.

The closure shouldn't become @MainActor isolated implicitly just because it's called from a @MainActor method.

Environment

swift-driver version: 1.112.3 Apple Swift version 6.0 (swiftlang-6.0.0.6.8 clang-1600.0.23.1)
Target: arm64-apple-macosx14.0

Additional information

No response

NachoSoto avatar Jul 24 '24 21:07 NachoSoto

cc @hborla

NachoSoto avatar Jul 24 '24 21:07 NachoSoto

For what it's worth, this exact situation is covered here in the migration guide:

https://www.swift.org/migration/documentation/swift-6-concurrency-migration-guide/incrementaladoption#Unmarked-Sendable-Closures

mattmassicotte avatar Jul 24 '24 21:07 mattmassicotte

Yes, this is a deliberate language design choice. I was leaving this open to try to dig up documentation for why, and if it doesn't exist, I'll write it up.

hborla avatar Jul 24 '24 22:07 hborla

This code will compile without issue but crash at runtime.

So you know that this would lead to a runtime crash and still decide to not produce a warning or error? 🫣

NachoSoto avatar Jul 24 '24 22:07 NachoSoto

So you know that this would lead to a runtime crash and still decide to not produce a warning or error? 🫣

I greatly appreciate your constructive feedback and your informative bug reports, but this sort of response isn't helpful. Please at least give me a chance to write up why the design decision was made.

hborla avatar Jul 24 '24 22:07 hborla

Apologies, I had understood that this was by design and was very confused about this decision.

Per https://www.swift.org/migration/documentation/swift-6-concurrency-migration-guide/incrementaladoption/#Unmarked-Sendable-Closures, adding @Sendable in would fix this, but it's not feasible to go through the entire codebase and find instances where this is needed to prevent crashes if the compiler can't at last warn us about it.

IMO, considering that Swift still doesn't have a way to silence individual warnings (and I'd like to continue compiling with warnings as errors), I would love for this behavior to not crash and also not produce a warning.

NachoSoto avatar Jul 24 '24 23:07 NachoSoto

So just to clarify, there is no warning this could happen? It could happen with any app that links with a Swift 6 module?

realityworks avatar Jul 25 '24 06:07 realityworks

If it's known that it will cause a runtime crash (which it seems to be) then it really should be an error that can be resolved by marking the closure as @Sendable as the migration doc says. Without an error it's going to be very hard to find where this occurs in any app other than sample or toy apps.

it makes incremental adoption difficult because this will introduce crashes in code that hasn't changed. So there would be no way to incrementally adopt and test an app. The whole app will need to be tested when just one module is updated to Swift 6. That's quite a big ask for commercial apps.

Jon889 avatar Jul 25 '24 16:07 Jon889

Perhaps my comment was misunderstood. The compiler does not and cannot know the code will crash at runtime, because it cannot see across module boundaries into the implementation of the function that contains the violation. My comment here:

Yes, this is a deliberate language design choice.

Means that the design choice to assume that non-Sendable closures don't leave the isolation domain they're formed in is deliberate, and it's valuable for eliminating the annotation burden in the long run. I want to elaborate on why, but that'll take me some time to articulate.

There is no need to pile onto this issue. I understand that people are struggling with runtime crashes that they can't predict, and I'm listening to the feedback. I'm not saying that no improvement is possible, but the solution is not as obvious as you all are making it out to be.

hborla avatar Jul 25 '24 17:07 hborla

I would love for this behavior to not crash and also not produce a warning.

You can disable the dynamic actor isolation checking with -disable-dynamic-actor-isolation.

hborla avatar Jul 25 '24 17:07 hborla

Thanks @hborla. I understand that this is by no means a simple problem with a simple solution, but I look forward to seeing what you all come up with 🙏🏻

NachoSoto avatar Jul 25 '24 18:07 NachoSoto

You can disable the dynamic actor isolation checking with -disable-dynamic-actor-isolation.

Is there a way to pass that through SPM?

NachoSoto avatar Jul 25 '24 18:07 NachoSoto

You can disable the dynamic actor isolation checking with -disable-dynamic-actor-isolation.

Is there a way to pass that through SPM?

I think something like this would possibly work?

.target(
    name: "...",
    swiftSettings: [.unsafeFlags(["-Xfrontend", "-disable-dynamic-actor-isolation"])]
)

freak4pc avatar Jul 25 '24 21:07 freak4pc

You can disable the dynamic actor isolation checking with -disable-dynamic-actor-isolation.

Is there a way to pass that through SPM?

I think something like this would possibly work?

.target(
    name: "...",
    swiftSettings: [.unsafeFlags(["-Xfrontend", "-disable-dynamic-actor-isolation"])]
)

To my knowledge, this would disallow the package from being used as a versioned dependency in other packages. It could be used as a revision dependency though: https://forums.swift.org/t/swift-package-manager-doesnt-check-for-unsafe-flags-when-the-dependency-uses-revision/73281

nmggithub avatar Jul 26 '24 03:07 nmggithub

I just want to make absolutely sure that everyone reading here understands that by disabling this runtime check you are explicitly opting into isolation violations. Stuff is going to get accessed on the the wrong thread.

mattmassicotte avatar Jul 26 '24 14:07 mattmassicotte

Should the compiler not infer actor isolation in this closure? I don't understand why it's:

  • Implicitly becoming @MainActor
  • Allowed to be passed as a parameter to a function that doesn't take a @MainActor closure

And to make it worse:

  • Crash with dispatch_queue_assert instead of producing a runtime warning.

NachoSoto avatar Jul 26 '24 14:07 NachoSoto

Should the compiler not infer actor isolation in this closure?

The compiler is currently following the rules as the language defines them. I think this issue here is the implication of how these rules interact with code that was compiled without them.

mattmassicotte avatar Jul 26 '24 15:07 mattmassicotte

The compiler is currently following the rules as the language defines them.

@mattmassicotte do you mind linking where the implicit behavior is documented, for future reference? (it's not easy to navigate Swift Concurrency rules)

krzyzanowskim avatar Aug 13 '24 06:08 krzyzanowskim

Just ran into another example of this that wasn't caught at compile time, but crashed at runtime:

image

If I hadn't noticed this, we could have shipped an instant crash-loop to our users, due to an issue that should have been detected at compile time, but instead it's a runtime dispatch_assert_queue crash.

This is completely unacceptable and I truly hope Swift 6 does not ship in this state.

NachoSoto avatar Aug 14 '24 18:08 NachoSoto

The compiler is currently following the rules as the language defines them.

@mattmassicotte do you mind linking where the implicit behavior is documented, for future reference? (it's not easy to navigate Swift Concurrency rules)

I think the most relevant proposal is SE-0302: Sendable and @Sendable closures.

mattmassicotte avatar Aug 14 '24 19:08 mattmassicotte

do you mind linking where the implicit behavior is documented, for future reference? (it's not easy to navigate Swift Concurrency rules)

I added documentation for this behavior in the migration guide here: https://www.swift.org/migration/documentation/swift-6-concurrency-migration-guide/dataracesafety#Function-Types

I think the text here can still be improved, but the philosophy is that the compiler shouldn't consider code to be concurrent until you explicitly opt into using concurrency (e.g. using annotations like @Sendable). By default, code is synchronous, and closures that are formed cannot be run concurrently. The problem in this GitHub issue is purely an incremental migration problem. The Swift 6 language mode will prevent you from passing a closure over an isolation boundary unless that closure is @Sendable. To catch actor isolation violations in code you don't own and that the compiler cannot see (i.e. because the implementation is across a module boundary), that's when the dynamic actor isolation checking from SE-0423 kicks in.

I also filed these two enhancements for the dynamic actor isolation checking to provide more actionable guidance when you hit one of these assertions, and to eliminate false-positives when the closure does not touch any actor-isolated state or methods in its body:

  1. https://github.com/swiftlang/swift/issues/75508
  2. https://github.com/swiftlang/swift/issues/75856

hborla avatar Aug 14 '24 19:08 hborla

Any updates here? We'd love to start shipping our app with Xcode 16 as soon as possible, but this bug makes it impossible for us to ship with confidence, as we know we'd be releasing dozens of unknown crashes.

NachoSoto avatar Sep 09 '24 20:09 NachoSoto

No, I do not have any updates on this issue. As always, I will post here when I have an update to share.

We'd love to start shipping our app with Xcode 16 as soon as possible, but this bug makes it impossible for us to ship with confidence, as we know we'd be releasing dozens of unknown crashes.

I assume you specifically mean with Swift 6 language mode adoption, right? Note that this issue does not impact projects built with the Swift 6.0 compiler in the Swift 5 language mode, even if you are building with -strict-concurrency=complete. This issue only impacts projects built with the DynamicActorIsolation upcoming feature or the -enable-actor-data-race-checks debugging flag enabled, and neither of these flags are automatically enabled under complete concurrency checking.

hborla avatar Sep 09 '24 20:09 hborla

I assume you specifically mean with Swift 6 language mode adoption, right? Note that this issue does not impact projects built with the Swift 6.0 compiler in the Swift 5 language mode,

We are seeing dispatch_assert_queue crashes even when compiling using Swift 5.

We just had to fix another one that would have unknowingly shipped to production affecting many millions of users: Screenshot 2024-09-19 at 10 18 49

So as is, we are unable to migrate to Xcode 16 even with Swift 5, which is very disappointing.

NachoSoto avatar Sep 19 '24 17:09 NachoSoto

Note that this issue does not impact projects built with the Swift 6.0 compiler in the Swift 5 language mode, even if you are building with -strict-concurrency=complete. This issue only impacts projects built with the DynamicActorIsolation upcoming feature or the -enable-actor-data-race-checks debugging flag enabled, and neither of these flags are automatically enabled under complete concurrency checking.

Does .enableExperimentalFeature("StrictConcurrency") enable that by any chance?

I'm pretty confused why this is still considered an "upcoming feature" in Xcode 16: Screenshot 2024-09-19 at 10 21 35

NachoSoto avatar Sep 19 '24 17:09 NachoSoto

A coworker ran into another one when in Swift 5 mode: image

It is so frustrating that in 2024, due to a known limitation in the language that was knowingly shipped, we have runtime crashes for something that could be detected at compile time. I thought Swift 6 was supposed to provide safe concurrency, but in reality this is making things much worse.

NachoSoto avatar Sep 20 '24 00:09 NachoSoto

FYI, to make matters worse, this is affecting apps on iOS 18 even without compiling with Swift 6, we see production crashes from system frameworks being affected by the same thing: Screenshot 2024-09-23 at 16 19 56

NachoSoto avatar Sep 23 '24 23:09 NachoSoto

FYI, to make matters worse, this is affecting apps on iOS 18 even without compiling with Swift 6, we see production crashes from system frameworks being affected by the same thing: Screenshot 2024-09-23 at 16 19 56

How is this even possible? Surely we're not going to run into these issue unless we compile with Swift 6.0?!?

realityworks avatar Sep 23 '24 23:09 realityworks

this is affecting apps on iOS 18 even without compiling with Swift 6, we see production crashes from system frameworks being affected by the same thing

I am not certain that this is the same problem, because I do not see the concurrency runtime executor check in that stack trace. That looks like a regular call to dispatch_assert_queue, which is also how dispatch implements the SerialExecutor.checkIsolated() protocol requirement, but it doesn't mean that all failures are coming from the compiler-injected isolation assertions. Please file a separate issue with the full crash log.

I know that you are frustrated, but continuously commenting on this issue is not going to accelerate the design and engineering work necessary to solve this issue.

hborla avatar Sep 23 '24 23:09 hborla

I am not certain that this is the same problem, because I do not see the concurrency runtime executor check in that stack trace. That looks like a regular call to dispatch_assert_queue

I confirmed that this is a regular call to dispatch_assert_queue written in Objective-C source. This is not related to Swift. You can get the same assertion failure entirely in Objective-C by invalidating this object off the main queue.

hborla avatar Sep 24 '24 11:09 hborla