-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added @MainActor to SharedSequence function arguments #2613
base: main
Are you sure you want to change the base?
Conversation
I think instead of adding 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 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 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 |
Thanks for the detailed reply @fabianmuecke!
I totally understand that, since I have been facing similar issues in my working projects. 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: |
Yeah, if we want to make RxSwift be at least publicly compliant, this needs more work. Especially all Fortunately the strict concurrency compilation will simply tell you to add |
I also believe that scheduler's should be deprecated, and most observables should have their closures marked as async and sendable. For example, if I am using .flatMap, chances are I am firing off another observable, which is very likely to be something that is running async. In an async context, if I want to switch thread contexts, I can simply mark the async closure as main actor or some other global actor. And for things that are inherently main threaded, like drivers, you can simply mark all of their closures as being main actor. |
I disagree with that. The power of Rx is that it isn't necessarily async. For example, all of my test code is synchronous, even if the same code is asynchronous in production. |
This is a very deep topic but in essence there are a few points to consider. First, don't necessarily take anything I say here as a 100% fact, this is my current understanding, and if someone has a vision they're passionate about, I'll be happy to see a more concrete POC of how that'd look. First about what I perceive as the consumer crowd of RxSwift, as of 2024:
About the relation to Swift Concurrency and "The Future":
So the gist here is that it still seems like uncharted territory to me, with many of its own issues (still). I view RxSwift as an extremely well thought-out and battle-tested framework/project that aims to provide cross-platform compatibility and adherence to the Reactive Extensions standard. There are no plans to do any major diversions from this definition. I'm happy to consider adding general quality-of-life improvements that would make sense (i.e. possibly If you have other opinions I'd be happy to hear them and discuss. Cheers 🤘 |
Maybe some more bridging APIs would be nice. Today theres the Maybe something like |
You can do asSingle().value to go into a single awaitable value I agree we miss some variations of converting async values / streams into observables, definitely a good addition for the next version |
The Scheduler is foundation in ReactiveX |
Agree on what @freak4pc said, personally think RxSwift is a different approach from Swift Concurrency. We can provide adoption to Swift 6, which is the language enforcement/enhancement(?), but we should not remove something that is originally in RxSwift Core. |
FYI - We're adding a |
Nice I look forward to this. |
@freak4pc Did you manage to get that working with test schedulers? I've added similar functions (Single.create, mapAsync etc.) as extensions to our app's codebase and never got it to play nicely with Rx schedulers (might actually be impossible because of how different the approaches are, I'm just curious). |
I haven't attempted specifically but it's an interesting question. I'm unsure if it can work because of the interplay between actors and schedulers. If you want to play with 6.8.0 and let me know what are the issues, I can try and poke a bit. |
I believe the issue was: the RxTest scheduler expects the scheduled actions to run synchronous, but a Task will run async, so collection of the values is finished, before the Task's closure finishes. Try this simple test: func testExample() {
let scheduler = TestScheduler(initialClock: 0)
let observer = scheduler.createObserver(Int.self)
let single = Single.create(work: {
try await Task.sleep(for: .seconds(1))
return 3
})
_ = single.asObservable().subscribe(observer)
scheduler.start()
XCTAssertEqual(observer.events, [.next(0, 3), .completed(0)])
} It'll fail because the events array is empty. Of course for such a simple case you can just do something like |
I think this is expected, as you are trying to requesting some async result synchronously.
I think provide async closure support to |
@tommyming I agree, an async Single.create function is necessary (as is an async map, which essentially just wraps such Single and flatMaps over it), that's why we have it anyway in our codebase. I found a way to workaround the testing issues implementing a wrapper around TestScheduler and adding a custom RxNimble-like implementation for collecting the results in an async fashion. That won't work for all edge-cases (especially the virtual timestamp of the events isn't really helpful anymore), but works for all things we need to test here. I just wish there was a way to integrate it properly with the existing concept of schedulers in RxSwift, so that no workarounds were necessary. But to make that work, I believe schedulers would have to take async closures, which would not only be a lot of work, but also introduce a whole bunch of new problems, I expect. TLDR; I didn't mean to suggest removing the Single.create function. I just wanted to raise awareness that this breaks the current testing approach using test schedulers. |
Since we can't guarantee that all |
@AndreiArdelean1 I think I tried that at first, but it was getting the compiler confused, leaving the non-mainactor implementations and adding it limited to Driver/Signal. Then I noticed, that we can guarantee it at the moment, because the SharedSequence initializer is internal, hence nobody outside the package can add another SharedSequence type than Signal and Driver, which are both guaranteed to run on the main thread. Or am I missing something? |
A |
We could add a Down is a sample code.
|
That wouldn't solve the issue, that the compiler wouldn't know whether to use the main actor isolated function overload or the one without it. Edit: Unless you call the overloads with |
I've checked with the code ⬇️ and if the closure needs to be on the MainActor, or if you manually specify
|
Interesting. That's been improved then (I tried with an Xcode beta, back when I added this PR). When I tried that it didn't even compile. In that case your suggestion is of course the better alternative, as it's future-proof, in case a new SharedSequence should be added, which isn't main actor bound. |
…g closure arguments.
…h are not limited to the main actor.
293806b
to
20a894b
Compare
@AndreiArdelean1 Updated the PR to integrate your suggestion of introducing the |
I don't know how the duplications can be avoided. Regarding the PR, I don't think |
I don't think it's necessary, no. I was playing it safe, because I noticed that some |
To get started on #2586 this PR adds
@preconcurrency @MainActor
to functions ofSharedSequence
,Driver
, andSignal
, 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 customSharedSequence
type, which does not use theMainScheduler
. I tried to find a way to limit this toSharingStrategy
s, which use theMainScheduler
, 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 bySharingStrategy
type and@MainActor
addition limited toSignalSharingSharingStrategy
, andDriverSharingStrategy
, so it doesn't break possibly existing custom implementations ofSharingStrategyProtocol
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 usingMainScheduler
).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.