From f927853f4a5a76553fedc8e41b0c26632f87bb83 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 12 Apr 2022 15:39:11 -0400 Subject: [PATCH] Bugfix: Fix race condition between interleaved and non-interleaved updates (#24353) * Regression test: Interleaved update race condition Demonstrates the bug reported in #24350. * Bugfix: Last update wins, even if interleaved "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. --- .../src/ReactFiberInterleavedUpdates.new.js | 4 ++ .../src/ReactFiberInterleavedUpdates.old.js | 4 ++ .../src/ReactFiberWorkLoop.new.js | 13 ++++- .../src/ReactFiberWorkLoop.old.js | 13 ++++- .../__tests__/ReactInterleavedUpdates-test.js | 48 +++++++++++++++++++ 5 files changed, 78 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js index 65460c7658abc..010730b1e78e0 100644 --- a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js +++ b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.new.js @@ -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 diff --git a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.old.js b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.old.js index 5fd769684ebd6..0d3319801daa6 100644 --- a/packages/react-reconciler/src/ReactFiberInterleavedUpdates.old.js +++ b/packages/react-reconciler/src/ReactFiberInterleavedUpdates.old.js @@ -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 diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index b6e7fad4eb944..3f0d3278f488b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -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, @@ -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 diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 62714c8cd3347..7dbbc9b25f9a7 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -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, @@ -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 diff --git a/packages/react-reconciler/src/__tests__/ReactInterleavedUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactInterleavedUpdates-test.js index c9e66fe0399e5..153d4d28bd3f7 100644 --- a/packages/react-reconciler/src/__tests__/ReactInterleavedUpdates-test.js +++ b/packages/react-reconciler/src/__tests__/ReactInterleavedUpdates-test.js @@ -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 ( + <> + + + + + ); + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + 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'); + }); });