Mark Operator Closures as @Sendable

[AndreiArdelean1]

Mark Operator Closures as @sendable to Prevent Crashes in Swift 6 Isolated Contexts #2638

In Swift 6, closures created in an isolated context automatically inherit the isolation of that context, unless they are marked as @sendable. Because of this, creating an operation like map, flatMap, distinctUntilChanged, etc. from an isolated context, but calling it from a different thread, causes the call to crash.

[AndreiArdelean1]

Added PR #2610 here

[AndreiArdelean1]

@freak4pc Hi! Just checking in to see if there’s any chance this PR could be reviewed. Getting this merged is essential for updating my project to Swift 6, and I imagine others may be in a similar situation. I’m eager to get feedback or make any adjustments needed to move it forward. Thanks so much for your time!

[freak4pc]

Hey @AndreiArdelean1 -
It's a very large change. Something that could speed up the mergeability of it is to reduce risk -
If you are willing to cut a release of your own app that targets this branch and confirm there are no production-related issues (crashes, misbehaviors) it would make it much simpler, since it will then only be a code review, and not a functionality review which would be very problematic to do without real-world tests on this.

[jjatie]

Rather than making these @Sendable we should make them sending to enable region based isolation, as it is easier for consumers of the API to provide sending closure than an @Sendable one.

Here is the relevant forum thread.

[jjatie]

Many Sendable conformances are unsafe, especially the generic ones. Rather than marking the generic types as @unchecked Sendable, which will cause issues when their generic types are non-Sendable. Instead, they should be conditionally Sendable, i.e.:

extension GenericClass: Sendable where Element1: Sendable, Element2: Sendable /* ... */ {}

Many (most?) of the class conformance's do not need to be marked @unchecked. From the Sendable documentation:

To satisfy the requirements of the Sendable protocol, a class must:

• Be marked final
• Contain only stored properties that are immutable and sendable
• Have no superclass or have NSObject as the superclass

Classes marked with @mainactor are implicitly sendable, because the main actor coordinates all access to its state. These classes can have stored properties that are mutable and nonsendable.

Classes that don’t meet the requirements above can be marked as @unchecked Sendable, disabling compile-time correctness checks, after you manually verify that they satisfy the Sendable protocol’s semantic requirements.

The closure typealiases do not need to be marked as @Sendable, only their usage as parameters need to be marked as sending

[jjatie]

Why do these need to be nonisolated?

[AndreiArdelean1]

Because elements is accessed both in the function body and in the subscribe closure. The subscribe closure is not necessarily executed in sync or on the same thread as materializeResult

[jjatie]

I don't believe this needs to be @Sendable, the parameter can simply be marked as sending

[AndreiArdelean1]

Commented on the PR:

From what I understand, sending is to be used only when transferring between isolated contexts. Most classes in RxSwift are not isolated, and shouldn't be and thus sending keyword is not ideal. Also sending can not be applied on closure property, even though they may be called from multiple threads and therefore should be sendable.

[jjatie]

Why is this unchecked? If this is going to be marker as Sendable, then it should be conditionally Sendable based on it's parameters:

extension GroupedObservable: Sendable where Key: Sendable, Element: Sendable {}

[AndreiArdelean1]

I would argue that Key and Element should be Sendable, but it's be too large of a change for now.

public struct GroupedObservable<Key: Sendable, Element: Sendable> : ObservableType, Sendable {

[jjatie]

No need to make this @unchecked

[AndreiArdelean1]

Yes because DisposeKey and SynchronizedUnsubscribeType are not currently marked as Sendable.

[jjatie]

Why does this need to be a requirement of the protocol?

[AndreiArdelean1]

All Schedulers in RxSwift are threadsafe and thus can be safely marked as Sendable.

As an example, when creating an observable with an .observe(on: scheduler) from inside a sendable closure, the compiler complains that scheduler is not sendable.

[jjatie]

I don't believe this needs to be @unchecked

[AndreiArdelean1]

it needs to be @unchecked because class Observable is not final

[jjatie]

Why does this need to be a requirement of the protocol?

[AndreiArdelean1]

All classes that conform to ObservableConvertibleType should be threadsafe and thus should be marked as Sendable.

[jjatie]

These don;'t need to be @Sendable, but their use as parameters should be marked as sending

[AndreiArdelean1]

defining

public static var defaultErrorHandler: sending DefaultErrorHandler {

gives the error 'sending' may only be used on parameters and results so it can't be added here.

assigning defaultErrorHandler from a main actor isolated context, without marking the closure as Sendable, and calling it from a non-main thread, would cause a runtime crash in Swift 6.

@MainActor
func test() {
    Hooks.defaultErrorHandler = { _, _ in }
}
test()
DispatchQueue.global().async(execute: { () in Hooks.defaultErrorHandler([], RxError.unknown) })

So it is safer to have the DefaultErrorHandler closure as sendable

[jjatie]

(Meant to mark my previous comment as Request Changes)

[AndreiArdelean1]

Rather than making these @Sendable we should make them sending to enable region based isolation, as it is easier for consumers of the API to provide sending closure than an @Sendable one.

Here is the relevant forum thread.

From what I understand, sending is to be used only when transferring between isolated contexts. Most classes in RxSwift are not isolated, and shouldn't be and thus sending keyword is not ideal. Also sending can not be applied on closure property, even though they may be called from multiple threads and therefore should be sendable. Even for example DispatchQueue.global().async requires the closures to be sendable.

Many Sendable conformances are unsafe, especially the generic ones. Rather than marking the generic types as @unchecked Sendable, which will cause issues when their generic types are non-Sendable. Instead, they should be conditionally Sendable, i.e.:

Most RxSwift classes should be Sendable. Let's take PublishSubject<Element> as an example. The element should always be Sendable since the class can't and shouldn't limit the context isolation (or thread) from which the element will be called. But for now, it is too restrictive and it would be too big of an impact on developers to adopt it, but it should be addressed in future versions. Thus, the sendability of PublishSubject shouldn't be conditioned by the sendability of Element as Element should always be sendable. If there are sendability related issues in PublishSubject<Element> because of Element, they are already in the code and are not introduced by @unchecked Sendable.

Ideally would be to use Sendable instead of @unchecked Sendable, but in some cases, it can't be done, or not right now. For example where we don't want a class to be final.

[freak4pc]

Looking at what happens with Combine my current instinct is consumers of RxSwift should @preconcurrency import RxSwift. It doesn't seem like a feasible goal to make RxSwift (or Combine) to be complete-concurrency safe easily given the vastly different concurrency models being follows.

Happy for other thoughts on this, but wondering if it's a futile attempt to even try going down this path.

[AndreiArdelean1]

Looking at what happens with Combine my current instinct is consumers of RxSwift should @preconcurrency import RxSwift. It doesn't seem like a feasible goal to make RxSwift (or Combine) to be complete-concurrency safe easily given the vastly different concurrency models being follows.

Happy for other thoughts on this, but wondering if it's a futile attempt to even try going down this path.

@freak4pc I would agree with you, but the reason I added this changes is that in Swift 6 the code below causes a runtime crash on map and it is extremely difficult to pinpoint the exact place where map is defined to fix the issue.

@preconcurrency import RxSwift

@MainActor
func test() -> any Disposable {
    return Observable<Int>.just(1)
        // any non main thread scheduler
        .observe(on: RxSwift.SerialDispatchQueueScheduler(qos: .background))
        .map({ $0 + 1 })
        .subscribe()
}

When the map closure is called, it expects to be executed on main thread because it is inside a function marked with @MainActor. The solution is to call map as .map({ @Sendable in $0 + 1 })

[jjatie]

@freak4pc I'm not sure it's futile, but I am not sure it's worthwhile trying to bring RxSwift to the Swift 6. Presumably swift-async-algorithms and/or the standard library will grow to serve many of the applications RxSwift is used for out of the box.

I think it does make sense to introduce more annotations to make it easier for those to adopt without having to entirely opt-out with @preconcurrency. I think improved conveniences to transition between Rx/Swift Concurrency in the form of Observables/Traits conveniences and perhaps an "ActorScheduler" if feasible would go a long way in my company's multi-million line project that is heavily reliant on RxSwift today. Especially at larger companies it's difficult to negotiate exceptions to be able to write @preconcurrency import

That being said, I'm not sure this PR in particular helps to achieve improving a migration story. I am curious how @AndreiArdelean1 is running into issues.

[jjatie]

From what I understand, sending is to be used only when transferring between isolated contexts. Most classes in RxSwift are not isolated, and shouldn't be and thus sending keyword is not ideal. Also sending can not be applied on closure property, even though they may be called from multiple threads and therefore should be sendable. Even for example DispatchQueue.global().async requires the closures to be sendable.

@AndreiArdelean1

Sendable is also only useful when transferring between isolated contexts. The difference between the two is:

• Sendable annotates that a type is safe to move across isolation contexts because it is impossible to perform an unsynchronized mutatation.
• sending annotates that:
  • A parameter value may move into a new isolation domain
  • A return value will be moved out of the method's isolation domain into the caller's

All Sendable types can be safely passed to sending parameters or from sending returns. The benefit that sending adds on top of that is that if the compiler can prove a non-Sendable value is not used after being transferred to the isolation domain, then it can be safely transferred despite not being `Sendable. This SE-0414 goes into great detail here.

Given you desire to make many parameters Sendable, marking them as sending instead opens up them up to more Swift 6 use case.

[jjatie]

Most RxSwift classes should be Sendable.

I agree that they can be Sendable, but I'm not sure they necessarily should (even if just because of developer impact). It's been a long time since I've used RxSwift with types that aren't inherently Sendable, but IIRC RxSwift does a fair bit of work to ensure that instances are appropriately transferred across schedulers and so long as consumers aren't mutating an input from their calling thread, they are fine. This is equivalent to sending parameters and as long as the API to pump events into an subscription are marked as sending then it is not a requirement everything be Sendable.

[AndreiArdelean1]

@jjatie

The benefit that sending adds on top of that is that if the compiler can prove a non-Sendable value is not used after being transferred to the isolation domain, then it can be safely transferred despite not being `Sendable. This SE-0414 goes into great detail here.

The problem with sending is that in the case of RxSwift, this is not guaranteed.

Here is an example:

let subject = PublishSubject<Int>()
subject
    .map({ $0 + 1 })
    .subscribe()
    .disposed(by: disposeBag)

DispatchQueue.global().async(execute: { subject.onNext(0) })
DispatchQueue.global().async(execute: { subject.onNext(1) })

In this case, the closure of the map, can be called simultaneous from multiple threads. Neither the closure nor the returned value is guaranteed to only be "transferred" to a new isolation context, and only be called from one context at any given time, as the sending annotation expects. Because of this, the closure (and the returned value), should be Sendable not sending
The map sink does this (link to code):

let mappedElement = try self.transform(element)
self.forwardOn(.next(mappedElement))

There is no lock around the transform function (and there shouldn't be) to ensure is only called from one thread at any given time.

Ideally even the Element of Observable should be Sendable, but for now it may be too big of an impact on devs using RxSwift. This is why I opted not to add this constraint.

---

and so long as consumers aren't mutating an input from their calling thread, they are fine

On top of the above example, RxSwift shouldn't add this constraint. If devs want to still use non-Sendable values inside closures, they can do so by either using nonisolated(unsafe) annotation or an @unchecked Sendable wrapper (like the one from here)