Skip to content

Commit

Permalink
Bug: Update while suspended fails to interrupt
Browse files Browse the repository at this point in the history
This demonstrates 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 compone 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 e2681cf
Showing 1 changed file with 36 additions and 0 deletions.
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 e2681cf

Please sign in to comment.