ReactiveX / RxSwift

Reactive Programming in Swift
MIT License
24.32k stars 4.17k forks source link

Added @MainActor to SharedSequence function arguments #2613

Open fabianmuecke opened 1 month ago

fabianmuecke commented 1 month ago

To get started on https://github.com/ReactiveX/RxSwift/issues/2586 this PR adds @preconcurrency @MainActor to functions of SharedSequence, Driver, and Signal, which take function arguments and adds @MainActor to the according function argument signatures.

I added this for all according SharedSequence functions, although technically it would be possible, to create a custom SharedSequence type, which does not use the MainScheduler. I tried to find a way to limit this to SharingStrategys, which use the MainScheduler, but failed to check that properly at compile-time, because the scheduler can be exchanged for a mock.

This means that:

[!IMPORTANT] Before this gets merged, the SharedSequence functions should probably be separated by SharingStrategy type and @MainActor addition limited to SignalSharingSharingStrategy, and DriverSharingStrategy, so it doesn't break possibly existing custom implementations of SharingStrategyProtocol out there.

Alternatively we could introduce a MainSchedulerSharingStrategy marker protocol, which would allow to limit it to sharing strategies conforming to that protocol and allow for easier adoption in custom implementations of sharing strategies (which might also be using MainScheduler).

I didn't go the extra mile of doing anything like that, because I first wanted to hear, if you consider adding @preconcurrency to introduce @MainActor to the code base is the right direction to go.

So please let me know what you think.

tommyming commented 1 month ago

I think instead of adding @MainActor to every single function that is conformed to SharedSequence, how about adding Main Actor functionality to the MainScheduler?

Not sure if that's possible, but it seems to be more reasonable and more elegant.

Feel free to correct me if I am wrong, or share your thoughts!

fabianmuecke commented 1 month ago

I think instead of adding @MainActor to every single function that is conformed to SharedSequence, how about adding Main Actor functionality to the MainScheduler?

Not sure if that's possible, but it seems to be more reasonable and more elegant.

Feel free to correct me if I am wrong, or share your thoughts!

The problem is, that there's no way to transport the fact that anything running on the MainScheduler is being performed on the MainActor. Each function has its own signature. I can see no way to apply @MainActor to all of them without either specifying it for each function individually or isolating the whole type to @MainActor.

I've been experimenting with adding an actor type to the type signature to solve this, so the actor isolation could be passed up and down the chaing and changed by using observe(on:) or subscribe(on:). This would require a scheduler to provide an actor type.

I failed at getting this implemented, because even if you do add an actor there, it does not add said actor to the function signatures. The only way to add that type to the function signatures would be to use add the actor type's instance to its signature like this (pseudo):

SharedSequence<SharingStrategy, Element, ActorType: Actor> {
    func map<NewElement>(_ transform: (isolated ActorType, Element) -> NewElement) -> SharedSequence<SharingStrategy, Element, ActorType> {
    …
    }
}

The problem here is: even if ActorType is MainActor, this does not (and cannot) add the @MainActor attribute, which is required by the Swift compiler to be able to run things marked as @MainActor isolated. There's a difference between the attribute and the type, because theoretically there could be multiple instances, even of a global actor type, so just having an actor of that type isn't good enough. Does that make any sense? It's hard to explain. 🥴

tommyming commented 1 month ago

Thanks for the detailed reply @fabianmuecke!

The problem here is: even if ActorType is MainActor, this does not (and cannot) add the @MainActor attribute, which is required by the Swift compiler to be able to run things marked as @MainActor isolated. There's a difference between the attribute and the type, because theoretically there could be multiple instances, even of a global actor type, so just having an actor of that type isn't good enough.

I totally understand that, since I have been facing similar issues in my working projects. Have been working on Swift 6 migration preparation recently, trying to adding specific GlobalActors to different services and layers, turns out is does not work as intended. The Swift compiler will just throwing errors at you about the actor isolation issue, which is the same issue that you are facing.

That's why I am asking is it possible to perform the same way in RxSwift.

In this case, I personally think using attributes this is a possble way to handle it properly.

side note: I think we need to take Swift 6 migration more seriously, I am skeptical how the strict concurrency mode will affect RxSwift. More investigation is needed.

fabianmuecke commented 1 month ago

Thanks for the detailed reply @fabianmuecke!

The problem here is: even if ActorType is MainActor, this does not (and cannot) add the @mainactor attribute, which is required by the Swift compiler to be able to run things marked as @mainactor isolated. There's a difference between the attribute and the type, because theoretically there could be multiple instances, even of a global actor type, so just having an actor of that type isn't good enough.

I totally understand that, since I have been facing similar issues in my working projects. Have been working on Swift 6 migration preparation recently, trying to adding specific GlobalActors to different services and layers, turns out is does not work as intended. The Swift compiler will just throwing errors at you about the actor isolation issue, which is the same issue that you are facing.

That's why I am asking is it possible to perform the same way in RxSwift.

In this case, I personally think using attributes this is a possble way to handle it properly.

side note: I think we need to take Swift 6 migration more seriously, I am skeptical how the strict concurrency mode will affect RxSwift. More investigation is needed.

Yeah, if we want to make RxSwift be at least publicly compliant, this needs more work. Especially all Elements will need to conform to Sendable, anything using internal thread safety mechanisms (like Subjects, Relay, etc.) should be marked as @unchecked Sendable and lots of other stuff.

Fortunately the strict concurrency compilation will simply tell you to add @preconcurrency to your import of frameworks like RxSwift or Combine, which will silence the compiler issues - but of course you also lose all benefits that way.