Skip to content

Commit

Permalink
Fix: Update while suspended fails to interrupt (#26739)
Browse files Browse the repository at this point in the history
This fixes a bug with `use` where if you update a component that's
currently suspended, React will sometimes mistake it for a render phase
update.

This happens because we don't reset `currentlyRenderingFiber` until the
suspended is unwound. And with `use`, that can happen asynchronously,
most commonly when the work loop is suspended during a transition.

The fix is to make sure `currentlyRenderingFiber` is only set when we're
in the middle of rendering, which used to be true until `use` was
introduced.

More specifically this means clearing `currentlyRenderingFiber` when
something throws and setting it again when we resume work.

In many cases, this bug will fail "gracefully" because the update is
still added to the queue; it's not dropped completely. It's also
somewhat rare because it has to be the exact same component that's
currently suspended. But it's still a bug. I wrote a regression test
that shows a sync update failing to interrupt a suspended component.
  • Loading branch information
acdlite committed Apr 28, 2023
1 parent 540bab0 commit 18282f8
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 8 deletions.
8 changes: 6 additions & 2 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,9 @@ function renderWithHooksAgain<Props, SecondArg>(
//
// Keep rendering in a loop for as long as render phase updates continue to
// be scheduled. Use a counter to prevent infinite loops.

currentlyRenderingFiber = workInProgress;

let numberOfReRenders: number = 0;
let children;
do {
Expand Down Expand Up @@ -826,13 +829,14 @@ export function resetHooksAfterThrow(): void {
//
// It should only reset things like the current dispatcher, to prevent hooks
// from being called outside of a component.
currentlyRenderingFiber = (null: any);

// We can assume the previous dispatcher is always this one, since we set it
// at the beginning of the render phase and there's no re-entrance.
ReactCurrentDispatcher.current = ContextOnlyDispatcher;
}

export function resetHooksOnUnwind(): void {
export function resetHooksOnUnwind(workInProgress: Fiber): void {
if (didScheduleRenderPhaseUpdate) {
// There were render phase updates. These are only valid for this render
// phase, which we are now aborting. Remove the updates from the queues so
Expand All @@ -842,7 +846,7 @@ export function resetHooksOnUnwind(): void {
// Only reset the updates from the queue if it has a clone. If it does
// not have a clone, that means it wasn't processed, and the updates were
// scheduled before we entered the render phase.
let hook: Hook | null = currentlyRenderingFiber.memoizedState;
let hook: Hook | null = workInProgress.memoizedState;
while (hook !== null) {
const queue = hook.queue;
if (queue !== null) {
Expand Down
12 changes: 6 additions & 6 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -1504,7 +1504,7 @@ function resetWorkInProgressStack() {
} else {
// Work-in-progress is in suspended state. Reset the work loop and unwind
// both the suspended fiber and all its parents.
resetSuspendedWorkLoopOnUnwind();
resetSuspendedWorkLoopOnUnwind(workInProgress);
interruptedWork = workInProgress;
}
while (interruptedWork !== null) {
Expand Down Expand Up @@ -1563,10 +1563,10 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {
return rootWorkInProgress;
}

function resetSuspendedWorkLoopOnUnwind() {
function resetSuspendedWorkLoopOnUnwind(fiber: Fiber) {
// Reset module-level state that was set during the render phase.
resetContextDependencies();
resetHooksOnUnwind();
resetHooksOnUnwind(fiber);
resetChildReconcilerOnUnwind();
}

Expand Down Expand Up @@ -2337,7 +2337,7 @@ function replaySuspendedUnitOfWork(unitOfWork: Fiber): void {
// is to reuse uncached promises, but we happen to know that the only
// promises that a host component might suspend on are definitely cached
// because they are controlled by us. So don't bother.
resetHooksOnUnwind();
resetHooksOnUnwind(unitOfWork);
// Fallthrough to the next branch.
}
default: {
Expand Down Expand Up @@ -2383,7 +2383,7 @@ function throwAndUnwindWorkLoop(unitOfWork: Fiber, thrownValue: mixed) {
//
// Return to the normal work loop. This will unwind the stack, and potentially
// result in showing a fallback.
resetSuspendedWorkLoopOnUnwind();
resetSuspendedWorkLoopOnUnwind(unitOfWork);

const returnFiber = unitOfWork.return;
if (returnFiber === null || workInProgressRoot === null) {
Expand Down Expand Up @@ -3744,7 +3744,7 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
// same fiber again.

// Unwind the failed stack frame
resetSuspendedWorkLoopOnUnwind();
resetSuspendedWorkLoopOnUnwind(unitOfWork);
unwindInterruptedWork(current, unitOfWork, workInProgressRootRenderLanes);

// Restore the original properties of the fiber.
Expand Down
36 changes: 36 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactUse-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1580,4 +1580,40 @@ describe('ReactUse', () => {
</>,
);
});

test('regression test: updates while component is suspended should not be mistaken for render phase updates', async () => {
const getCachedAsyncText = cache(getAsyncText);

let setState;
function App() {
const [state, _setState] = useState('A');
setState = _setState;
return <Text text={use(getCachedAsyncText(state))} />;
}

// Initial render
const root = ReactNoop.createRoot();
await act(() => root.render(<App />));
assertLog(['Async text requested [A]']);
expect(root).toMatchRenderedOutput(null);
await act(() => resolveTextRequests('A'));
assertLog(['A']);
expect(root).toMatchRenderedOutput('A');

// Update to B. This will suspend.
await act(() => startTransition(() => setState('B')));
assertLog(['Async text requested [B]']);
expect(root).toMatchRenderedOutput('A');

// While B is suspended, update to C. This should immediately interrupt
// the render for B. In the regression, this update was mistakenly treated
// as a render phase update.
ReactNoop.flushSync(() => setState('C'));
assertLog(['Async text requested [C]']);

// Finish rendering.
await act(() => resolveTextRequests('C'));
assertLog(['C']);
expect(root).toMatchRenderedOutput('C');
});
});

0 comments on commit 18282f8

Please sign in to comment.