[FlatMapLatest] Fix crash and hang under a single consumer - #442
Open
kSibalic wants to merge 1 commit into
Open
Conversation
- `startInnerTask` mutated the state machine without holding the lock; a stale write-back could resurrect an already resumed downstream continuation (trapping in `next(for:)`) or erase a freshly stored one (hanging the consumer) - Hold the lock across inner task creation and registration so the task body cannot transition state before the task is registered, matching `startOuterTask` - `next(for:)` asserted state was unchanged across the unlock/relock window in `FlatMapLatestStorage.next()`; serve buffered elements or the finished state instead of trapping (apple#437) - Add a stress regression test based on the issue's reproduction
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Consuming a
flatMapLatestsequence with a singlefor awaitloop, where the base switches inner sequences frequently, can trap (#437):The same scenario also intermittently hangs; the downstream continuation is never resumed.
There are two root causes:
Unlocked state machine mutation:
FlatMapLatestStorage.startInnerTaskcallsstateMachine.innerTaskStarted(_:generation:)without holding the lock, every path to it runs afterlock.withLockhas returned. This "read-modify-write" of the whole state tuple races with locked transitions: a stale write-back can resurrect an already resumed downstream continuation (the trap) or erase a freshly stored one (the hang). It also allows an empty inner sequence to runinnerFinishedbefore the task is registered, leaving the state claiming an inner is running forever.Non-atomic suspension:
FlatMapLatestStorage.next()releases the lock beforesuspend()re-acquires it to callnext(for:), so producers can legitimately buffer elements or finish inside that window. The preconditions innext(for:)assumed atomicity that doesn't hold there.Modifications
FlatMapLatestStorage.startInnerTasknow holds the lock across task creation and registration. The task body's first action is acquiring that same lock, so it cannot transition the state machine until registration completes; the disciplinestartOuterTaskalready follows. After this change, every state machine access happens with the lock held.FlatMapLatestStateMachine.next(for:)serves the demand from the state it observes instead of trapping it pops a buffered element (or error), or finishes, before parking the continuation mirroringnext(). The single-consumer precondition (downstreamCont == nil) remains, as it is a true invariant once registration is locked.Result
The regression test doubles as the reproduction. Checking out only the test file from this branch onto
main, with the sources left unfixed, makes it fail in one of the two reported ways: a trap on thenext(for:)precondition ("Already have downstream continuation"), or a 120-second timeout because the downstream continuation was lost and the consumer is never resumed.Pre-fix failure output
With this change, the same test passes in about 2.5 seconds. The hang disappears because the inner task is now registered under the lock, so no stale write-back can erase a stored downstream continuation. The trap disappears for the same reason (no write-back can resurrect an already resumed continuation) and because
next(for:)now serves elements buffered during the legitimate unlock window instead of asserting that the window was empty.Resolves #437.