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
| try await withCheckedThrowingContinuation { continuation in | ||
| var didEmit = false | ||
| 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.
| Hi @freak4pc, sorry to ping you, is there anything else I need to do to get this in a mergeable state? We would love to stop maintaining a fork of RxSwift, but this had become our top crash in Goodnotes. |
| Thanks for the code, sorry for the delay, I was on vacation. Will try to cut a release with this soon. |
4 participants
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