Skip to content
Prev Previous commit
Next Next commit
Restart on suspend if return path has an update
Similar to the previous commit, if we suspend with a delay, and
something in the return path has a pending update, we should abort
the current render and switch to the update instead.
  • Loading branch information
acdlite committed Sep 10, 2019
commit 2d93c7eb8a439fa034c80d760c22627ccf875379
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -2064,7 +2064,7 @@ function updateDehydratedSuspenseComponent(
// render something, if we time out. Even if that requires us to delete everything and
// skip hydration.
// Delay having to do this as long as the suspense timeout allows us.
renderDidSuspendDelayIfPossible();
renderDidSuspendDelayIfPossible(workInProgress);
return retrySuspenseComponentWithoutHydrating(
current,
workInProgress,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -922,7 +922,7 @@ function completeWork(
} else {
// Otherwise, we're going to have to hide content so we should
// suspend for longer if possible.
renderDidSuspendDelayIfPossible();
renderDidSuspendDelayIfPossible(workInProgress);
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,14 @@ export function resetHooks(): void {
// This is used to reset the state of this module when a component throws.
// It's also called inside mountIndeterminateComponent if we determine the
// component is a module-style component.

if (currentlyRenderingFiber !== null) {
// Even though this component didn't complete, set the remaining time left
// on this fiber. This is sometimes useful when suspending to determine if
// there's a lower priority update that could "unsuspend."
currentlyRenderingFiber.expirationTime = remainingExpirationTime;
}

renderExpirationTime = NoWork;
currentlyRenderingFiber = null;

Expand Down
28 changes: 27 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,8 @@ function renderRoot(
thrownValue,
renderExpirationTime,
);
// TODO: This is not wrapped in a try-catch, so if the complete phase
// throws, we won't capture it.
workInProgress = completeUnitOfWork(sourceFiber);
}
} while (true);
Expand Down Expand Up @@ -1259,13 +1261,37 @@ export function renderDidSuspend(): void {
}
}

export function renderDidSuspendDelayIfPossible(): void {
export function renderDidSuspendDelayIfPossible(suspendedWork: Fiber): void {
if (
workInProgressRootExitStatus === RootIncomplete ||
workInProgressRootExitStatus === RootSuspended
) {
workInProgressRootExitStatus = RootSuspendedWithDelay;
}

if (workInProgressRoot !== null) {
// Check if the component that suspsended, or any components in the return
// path, have a pending update. If so, those updates might unsuspend us, so
// interrupt the current render and restart.
let nextAfterSuspendedTime = NoWork;
let fiber = suspendedWork;
while (fiber !== null) {
const updateExpirationTime = fiber.expirationTime;
if (updateExpirationTime > nextAfterSuspendedTime) {
nextAfterSuspendedTime = updateExpirationTime;
}
fiber = fiber.return;
}

if (nextAfterSuspendedTime !== NoWork) {
// Mark the current render as suspended, and then mark that there's a
// pending update.
// TODO: This should immediately interrupt the current render, instead
// of waiting until the next time we yield.
markRootSuspendedAtTime(workInProgressRoot, renderExpirationTime);
markRootUpdatedAtTime(workInProgressRoot, nextAfterSuspendedTime);
}
}
}

export function renderDidError() {
Expand Down
121 changes: 121 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,127 @@ describe('ReactSuspense', () => {
},
);

it(
'interrupts current render when something suspends with a ' +
"delay and we've already skipped over a lower priority update in " +
'a parent',
() => {
function interrupt() {
// React has a heuristic to batch all updates that occur within the same
// event. This is a trick to circumvent that heuristic.
ReactTestRenderer.create('whatever');
}

function App({shouldSuspend, step}) {
return (
<>
<Text text={`A${step}`} />
<Suspense fallback={<Text text="Loading..." />}>
{shouldSuspend ? <AsyncText text="Async" ms={2000} /> : null}
</Suspense>
<Text text={`B${step}`} />
<Text text={`C${step}`} />
</>
);
}

const root = ReactTestRenderer.create(null, {
unstable_isConcurrent: true,
});

root.update(<App shouldSuspend={false} step={0} />);
expect(Scheduler).toFlushAndYield(['A0', 'B0', 'C0']);
expect(root).toMatchRenderedOutput('A0B0C0');

// This update will suspend.
root.update(<App shouldSuspend={true} step={1} />);

// Need to move into the next async bucket.
Scheduler.unstable_advanceTime(1000);
// Do a bit of work, then interrupt to trigger a restart.
expect(Scheduler).toFlushAndYieldThrough(['A1']);
interrupt();

// Schedule another update. This will have lower priority because of
// the interrupt trick above.
root.update(<App shouldSuspend={false} step={2} />);

expect(Scheduler).toFlushAndYieldThrough([
// Should have restarted the first update, because of the interruption
'A1',
'Suspend! [Async]',
'Loading...',
'B1',
]);

// Should not have committed loading state
expect(root).toMatchRenderedOutput('A0B0C0');

// After suspending, should abort the first update and switch to the
// second update. So, C1 should not appear in the log.
// TODO: This should work even if React does not yield to the main
// thread. Should use same mechanism as selective hydration to interrupt
// the render before the end of the current slice of work.
expect(Scheduler).toFlushAndYield(['A2', 'B2', 'C2']);

expect(root).toMatchRenderedOutput('A2B2C2');
},
);

it(
'interrupts current render when something suspends with a ' +
'delay, and a parent received an update after it completed',
() => {
function App({shouldSuspend, step}) {
return (
<>
<Text text={`A${step}`} />
<Suspense fallback={<Text text="Loading..." />}>
{shouldSuspend ? <AsyncText text="Async" ms={2000} /> : null}
</Suspense>
<Text text={`B${step}`} />
<Text text={`C${step}`} />
</>
);
}

const root = ReactTestRenderer.create(null, {
unstable_isConcurrent: true,
});

root.update(<App shouldSuspend={false} step={0} />);
expect(Scheduler).toFlushAndYield(['A0', 'B0', 'C0']);
expect(root).toMatchRenderedOutput('A0B0C0');

// This update will suspend.
root.update(<App shouldSuspend={true} step={1} />);
// Flush past the root, but stop before the async component.
expect(Scheduler).toFlushAndYieldThrough(['A1']);

// Schedule an update on the root, which already completed.
root.update(<App shouldSuspend={false} step={2} />);
// We'll keep working on the existing update.
expect(Scheduler).toFlushAndYieldThrough([
// Now the async component suspends
'Suspend! [Async]',
'Loading...',
'B1',
]);

// Should not have committed loading state
expect(root).toMatchRenderedOutput('A0B0C0');

// After suspending, should abort the first update and switch to the
// second update. So, C1 should not appear in the log.
// TODO: This should work even if React does not yield to the main
// thread. Should use same mechanism as selective hydration to interrupt
// the render before the end of the current slice of work.
expect(Scheduler).toFlushAndYield(['A2', 'B2', 'C2']);

expect(root).toMatchRenderedOutput('A2B2C2');
},
);

it('mounts a lazy class component in non-concurrent mode', async () => {
class Class extends React.Component {
componentDidMount() {
Expand Down