Skip to content

Commit

Permalink
Bugfix: Last update wins, even if interleaved
Browse files Browse the repository at this point in the history
"Interleaved" updates are updates that are scheduled while a render is
already in progress. We put these on a special queue so that they don't
get processed during the current render. Then we transfer them to
the "real" queue after the render has finished.

There was a race condition where an update is received after the render
has finished but before the interleaved update queue had been
transferred, causing the updates to be queued in the wrong order.

The fix I chose is to check if the interleaved updates queue is empty
before adding any update to the real queue. If it's not empty, then
the new update must also be treated as interleaved.
  • Loading branch information
acdlite committed Apr 12, 2022
1 parent f05029d commit c372d03
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ export function pushInterleavedQueue(
}
}

export function hasInterleavedUpdates() {
return interleavedQueues !== null;
}

export function enqueueInterleavedUpdates() {
// Transfer the interleaved updates onto the main queue. Each queue has a
// `pending` field and an `interleaved` field. When they are not null, they
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ export function pushInterleavedQueue(
}
}

export function hasInterleavedUpdates() {
return interleavedQueues !== null;
}

export function enqueueInterleavedUpdates() {
// Transfer the interleaved updates onto the main queue. Each queue has a
// `pending` field and an `interleaved` field. When they are not null, they
Expand Down
13 changes: 11 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,10 @@ import {
pop as popFromStack,
createCursor,
} from './ReactFiberStack.new';
import {enqueueInterleavedUpdates} from './ReactFiberInterleavedUpdates.new';
import {
enqueueInterleavedUpdates,
hasInterleavedUpdates,
} from './ReactFiberInterleavedUpdates.new';

import {
markNestedUpdateScheduled,
Expand Down Expand Up @@ -723,7 +726,13 @@ export function isInterleavedUpdate(fiber: Fiber, lane: Lane) {
// TODO: Optimize slightly by comparing to root that fiber belongs to.
// Requires some refactoring. Not a big deal though since it's rare for
// concurrent apps to have more than a single root.
workInProgressRoot !== null &&
(workInProgressRoot !== null ||
// If the interleaved updates queue hasn't been cleared yet, then
// we should treat this as an interleaved update, too. This is also a
// defensive coding measure in case a new update comes in between when
// rendering has finished and when the interleaved updates are transferred
// to the main queue.
hasInterleavedUpdates() !== null) &&
(fiber.mode & ConcurrentMode) !== NoMode &&
// If this is a render phase update (i.e. UNSAFE_componentWillReceiveProps),
// then don't treat this as an interleaved update. This pattern is
Expand Down
13 changes: 11 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,10 @@ import {
pop as popFromStack,
createCursor,
} from './ReactFiberStack.old';
import {enqueueInterleavedUpdates} from './ReactFiberInterleavedUpdates.old';
import {
enqueueInterleavedUpdates,
hasInterleavedUpdates,
} from './ReactFiberInterleavedUpdates.old';

import {
markNestedUpdateScheduled,
Expand Down Expand Up @@ -723,7 +726,13 @@ export function isInterleavedUpdate(fiber: Fiber, lane: Lane) {
// TODO: Optimize slightly by comparing to root that fiber belongs to.
// Requires some refactoring. Not a big deal though since it's rare for
// concurrent apps to have more than a single root.
workInProgressRoot !== null &&
(workInProgressRoot !== null ||
// If the interleaved updates queue hasn't been cleared yet, then
// we should treat this as an interleaved update, too. This is also a
// defensive coding measure in case a new update comes in between when
// rendering has finished and when the interleaved updates are transferred
// to the main queue.
hasInterleavedUpdates() !== null) &&
(fiber.mode & ConcurrentMode) !== NoMode &&
// If this is a render phase update (i.e. UNSAFE_componentWillReceiveProps),
// then don't treat this as an interleaved update. This pattern is
Expand Down

0 comments on commit c372d03

Please sign in to comment.