Fix race conditions in PrimitiveSequence+Concurrency #2641
Conversation
This caused continuations to be resumed more than once, crashing clients due to a violation of the Swift Concurrency contract
| var didResume = false | ||
| let lock = RecursiveLock() |
There was a problem hiding this comment.
Take this with a grain of salt, as far as I can tell RecursiveLock is the to-go tool in this codebase.
- does it have to be recursive?
- can it be replaced with an atomic & compare-and-swap?
There was a problem hiding this comment.
Hey, that's a good point. I used RecursiveLock precisely because of that, I saw that was the tool being used to synchronize code in the rest of the codebase. I initially thought about that but given that even AtomicInt inside RxSwift is backed by a NSLock, I didn't want to introduce a different way to synchronize this.
If a maintainer wants me to switch to CAS for this I'm happy to do that.
There was a problem hiding this comment.
I think changing didResume to AtomicInt and using fetchOr might be better 🙏
There was a problem hiding this comment.
I don't think so, AtomicInt is backed by an NSLock so the only thing that would do is making this code less clear.
This caused continuations to be resumed more than once, crashing clients due to a violation of the Swift Concurrency contract.
I added tests that make it easy to reproduce the issue under the previous implementation
This should fix #2563