Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Fix race condition between interleaved and non-interleaved updates #24353

Merged
merged 2 commits into from
Apr 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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) &&
gaearon marked this conversation as resolved.
Show resolved Hide resolved
(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
Original file line number Diff line number Diff line change
Expand Up @@ -140,4 +140,52 @@ describe('ReactInterleavedUpdates', () => {
expect(Scheduler).toHaveYielded([2, 2, 2]);
expect(root).toMatchRenderedOutput('222');
});

test('regression for #24350: does not add to main update queue until interleaved update queue has been cleared', async () => {
let setStep;
function App() {
const [step, _setState] = useState(0);
setStep = _setState;
return (
<>
<Text text={'A' + step} />
<Text text={'B' + step} />
<Text text={'C' + step} />
</>
);
}

const root = ReactNoop.createRoot();
await act(async () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['A0', 'B0', 'C0']);
expect(root).toMatchRenderedOutput('A0B0C0');

await act(async () => {
// Start the render phase.
startTransition(() => {
setStep(1);
});
expect(Scheduler).toFlushAndYieldThrough(['A1', 'B1']);

// Schedule an interleaved update. This gets placed on a special queue.
startTransition(() => {
setStep(2);
});

// Finish rendering the first update.
expect(Scheduler).toFlushUntilNextPaint(['C1']);

// Schedule another update. (In the regression case, this was treated
// as a normal, non-interleaved update and it was inserted into the queue
// before the interleaved one was processed.)
startTransition(() => {
setStep(3);
});
});
// The last update should win.
expect(Scheduler).toHaveYielded(['A3', 'B3', 'C3']);
expect(root).toMatchRenderedOutput('A3B3C3');
});
});