Repository navigation
Avoid primitive sequence cancellation deadlock - #2727
victorpimentel wants to merge 2 commits into
Conversation
|
Hi @nikolaykasyanov and @freak4pc. Since you were involved in #2641, I thought you might have useful context on this follow-up. We are seeing this lock-ordering race frequently as an app hang in Goodnotes production. Any thoughts would be appreciated. Thanks! |
There was a problem hiding this comment.
thanks for the contribution!
not related to this PR, but same shape as what it fixes: values on ObservableConvertibleType still guards its terminal with a plain var isFinished, written from onError/onCompleted and read from onDisposed, no sync at all. finish() is idempotent so it's not a crash like the double-resume was, but it's a real race. probably wants the same AtomicInt latch. opening a follow-up.
|
|
||
| /// A previous implementation of the `Single` to swift concurrency bridge had a bug where it would sometimes call the continuation twice. | ||
| /// A previous implementation of the `Single` to swift concurrency bridge could call the continuation twice or deadlock when completion raced cancellation. | ||
| /// The current number of iterations is a sweet spot to not make the tests too slow while still catching the bug in most runs. |
There was a problem hiding this comment.
this line says the iteration count catches the bug in most runs, but per the description the deadlock only reproduced 5/30 on 6.10.2. thats a pretty weak guard for something thats hanging in prod. worth bumping iterations or tightening the emit/cancel window so it fails reliably against the old impl? at minimum the sentence should reflect the new odds. same copy in the maybe + completable docs.
There was a problem hiding this comment.
Yeah, I think 10k iterations were a compromise to avoid having very long test times. The changes in the tests right now do not guarantee failure in those cases but are much more stronger since now we wait for all blocks to finish exactly once.
However now the tests run a bit faster than before since we avoid sleeping too much, we can bump the number of iterations if you think it makes sense. What would be your accepted running time in an “average” machine?
There was a problem hiding this comment.
uhm i missed the fact it's already at 10k, let's leave it like this. youll probably need over 100,000 runs to make it a meaningful card which probably isn't worth it.
freak4pc
left a comment
freak4pc
left a comment
There was a problem hiding this comment.
if you want to take a look at the second place i suggested that'd be good, but not mandatory since a bit out of scope :) PR looks good to me otherwise.
|
I'll be able to take a look at it next week. Still, when do you think this change may get to a release? Thanks! |
* origin/main: Fix Observable values terminal race (ReactiveX#2728) Bump SPM watchOS deployment target to 5.0 (ReactiveX#2732) Bump faraday from 1.4.1 to 1.10.6 (ReactiveX#2720) Select Xcode 26.6 for CI on the macOS 26 runner image (ReactiveX#2726) Add regression test for the merge lock-inversion deadlock Run merge teardown outside forwardLock, keeping the disposed flag inside it Original fix of merge deadlock Add regression tests for the ReplaySubject, Connection and Zip lock-inversion deadlocks Apply review cleanups to Multicast and Zip Use performLocked and strip trailing whitespace Make ZipCollectionTypeSink not call out under lock Connect only outside of lock Make sure to never call dispose and subscribe underlock in Connection, ConnectionObservableAdapter and RefCountSink ReplayBufferBase propagate outiside locks Fix: missing CFBundleShortVersionString and update RX_VERSION *issue no ReactiveX#2687 (ReactiveX#2696) Fix concurrent share subscriptions (ReactiveX#2719) Bump addressable from 2.8.0 to 2.9.0 (ReactiveX#2702)
|
@freak4pc Do we need something else for this to be merged? Thank you! |
2 participants
Single.value,Maybe.value, andCompletable.valuecurrently resume their continuations while holding the bridge lock. If completion races task cancellation, that can invert the bridge and Swift task locks and deadlock.This is showing up frequently as an app hang in Goodnotes production.
This uses RxSwift's existing
AtomicIntlatch to select one callback, then resumes after the latch has released its lock. The race tests now wait for all 10,000 bridged tasks, so a stuck task cannot pass unnoticed. They are also faster: 1.87 seconds combined instead of 4.07 seconds, averaged across five isolated runs on my machine.On RxSwift 6.10.2, the updated tests timed out in 5 of 30 race runs on an iOS 26 simulator. With this change, the full test class passed 180 of 180 runs.
This follows up on #2641, which added the bridge lock to prevent continuations from being resumed more than once.