-
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
Allow VirtualTimeScheduler to run on any thread #2610
base: main
Are you sure you want to change the base?
Allow VirtualTimeScheduler to run on any thread #2610
Conversation
…ethods are all called on the same thread.
As far as I can tell, this is 100% backwards compatible and doesn't require any API change from users of the library. It may not solve every future use-case that Swift Testing might throw at it, but it solves the single most obvious one. I see no reason that this can't be accepted into the next minor release/update. |
guard thread == nil || Thread.current == thread else { | ||
rxFatalError("Executing on the wrong thread. Please ensure all work on the same thread.") | ||
} | ||
thread = Thread.current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to read and write thread
without any locking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that what we always do on the main thread? Read/write without locking? Maybe I don't understand the question...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it should be completely fine as long scheduler's methods are actually called on one thread, but what if there would be two close calls from different threads, it's not impossible that both will see thread == nil
and proceed, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! Good point. I will update to account for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikolaykasyanov What do you think about just assigning the thread in the init
? Then we won't need a lock...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielt1263 sounds good!
What's the hold up in getting this merged? Would love to start being able to migrate our RxSwift tests to Swift Testing @nikolaykasyanov @danielt1263 |
We'll need someone with a meaningful RxSwift-based test suite to test this to make sure it doesn't break existing tests. |
…as long as critical methods are all called on the same thread.
This is designed to fix issue #2609