From 65edc8d9835f378faba1c2cc6ca612d969fd1e1e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 17 Sep 2019 10:14:34 -0700 Subject: [PATCH 1/9] [scheduler][profiler] Start time of delayed tasks Fixes a bug in the Scheduler profiler where the start time of a delayed tasks is always 0. From 6d5a2c293bc638472a8eafb9bbfebf3529f43c12 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 16 Sep 2019 17:36:03 -0700 Subject: [PATCH 2/9] [suspense][error handling] Add failing unit test Covers an edge case where an error is thrown inside the complete phase of a component that is in the return path of a component that suspends. The second error should also be handled (i.e. able to be captured by an error boundary. The test is currently failing because there's a call to `completeUnitOfWork` inside the main render phase `catch` block. That call is not itself wrapped in try-catch, so anything that throws is treated as a fatal/unhandled error. I believe this bug is only observable if something in the host config throws; and, only in legacy mode, because in concurrent/batched mode, `completeUnitOfWork` on fiber that throws follows the "unwind" path only, not the "complete" path, and the "unwind" path does not call any host config methods. --- ...tSuspenseWithNoopRenderer-test.internal.js | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 97f0f480fbeb..6ed257726d8c 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -1392,6 +1392,40 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushExpired(['Hi']); }); } + + it('handles errors in the return path of a component that suspends', async () => { + // Covers an edge case where an error is thrown inside the complete phase + // of a component that is in the return path of a component that suspends. + // The second error should also be handled (i.e. able to be captured by + // an error boundary. + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error, errorInfo) { + return {error}; + } + render() { + if (this.state.error) { + return `Caught an error: ${this.state.error.message}`; + } + return this.props.children; + } + } + + ReactNoop.renderLegacySyncRoot( + + + + + + + , + ); + + expect(Scheduler).toHaveYielded(['Suspend! [Async]']); + expect(ReactNoop).toMatchRenderedOutput( + 'Caught an error: Error in host config.', + ); + }); }); it('does not call lifecycles of a suspended component', async () => { From 975636be7821cadfc682b902a09e862582cf58ec Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 16 Sep 2019 13:13:37 -0700 Subject: [PATCH 3/9] Outline push/pop logic in `renderRoot` I want to get rid of the the `isSync` argument to `renderRoot`, and instead use separate functions for concurrent and synchronous render. As a first step, this extracts the push/pop logic that happens before and after the render phase into helper functions. --- .../src/ReactFiberWorkLoop.js | 72 +++++++++++-------- 1 file changed, 41 insertions(+), 31 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index be45086f2f7f..4ea48c051853 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -1249,20 +1249,8 @@ function renderRoot( if (workInProgress !== null) { const prevExecutionContext = executionContext; executionContext |= RenderContext; - let prevDispatcher = ReactCurrentDispatcher.current; - if (prevDispatcher === null) { - // The React isomorphic package does not include a default dispatcher. - // Instead the first renderer will lazily attach one, in order to give - // nicer error messages. - prevDispatcher = ContextOnlyDispatcher; - } - ReactCurrentDispatcher.current = ContextOnlyDispatcher; - let prevInteractions: Set | null = null; - if (enableSchedulerTracing) { - prevInteractions = __interactionsRef.current; - __interactionsRef.current = root.memoizedInteractions; - } - + const prevDispatcher = pushDispatcher(root); + const prevInteractions = pushInteractions(root); startWorkLoopTimer(workInProgress); do { @@ -1312,16 +1300,47 @@ function renderRoot( workInProgress = completeUnitOfWork(sourceFiber); } } while (true); - - executionContext = prevExecutionContext; resetContextDependencies(); - ReactCurrentDispatcher.current = prevDispatcher; + executionContext = prevExecutionContext; + popDispatcher(prevDispatcher); if (enableSchedulerTracing) { - __interactionsRef.current = ((prevInteractions: any): Set); + popInteractions(((prevInteractions: any): Set)); } } } +function pushDispatcher(root) { + const prevDispatcher = ReactCurrentDispatcher.current; + ReactCurrentDispatcher.current = ContextOnlyDispatcher; + if (prevDispatcher === null) { + // The React isomorphic package does not include a default dispatcher. + // Instead the first renderer will lazily attach one, in order to give + // nicer error messages. + return ContextOnlyDispatcher; + } else { + return prevDispatcher; + } +} + +function popDispatcher(prevDispatcher) { + ReactCurrentDispatcher.current = prevDispatcher; +} + +function pushInteractions(root) { + if (enableSchedulerTracing) { + const prevInteractions: Set | null = __interactionsRef.current; + __interactionsRef.current = root.memoizedInteractions; + return prevInteractions; + } + return null; +} + +function popInteractions(prevInteractions) { + if (enableSchedulerTracing) { + __interactionsRef.current = prevInteractions; + } +} + export function markCommitTimeOfFallback() { globalMostRecentFallbackTime = now(); } @@ -1760,11 +1779,7 @@ function commitRootImpl(root, renderPriorityLevel) { if (firstEffect !== null) { const prevExecutionContext = executionContext; executionContext |= CommitContext; - let prevInteractions: Set | null = null; - if (enableSchedulerTracing) { - prevInteractions = __interactionsRef.current; - __interactionsRef.current = root.memoizedInteractions; - } + const prevInteractions = pushInteractions(root); // Reset this to null before calling lifecycles ReactCurrentOwner.current = null; @@ -1882,7 +1897,7 @@ function commitRootImpl(root, renderPriorityLevel) { requestPaint(); if (enableSchedulerTracing) { - __interactionsRef.current = ((prevInteractions: any): Set); + popInteractions(((prevInteractions: any): Set)); } executionContext = prevExecutionContext; } else { @@ -2151,18 +2166,13 @@ export function flushPassiveEffects() { } function flushPassiveEffectsImpl(root, expirationTime) { - let prevInteractions: Set | null = null; - if (enableSchedulerTracing) { - prevInteractions = __interactionsRef.current; - __interactionsRef.current = root.memoizedInteractions; - } - invariant( (executionContext & (RenderContext | CommitContext)) === NoContext, 'Cannot flush passive effects while already rendering.', ); const prevExecutionContext = executionContext; executionContext |= CommitContext; + const prevInteractions = pushInteractions(root); // Note: This currently assumes there are no passive effects on the root // fiber, because the root is not part of its own effect list. This could @@ -2193,7 +2203,7 @@ function flushPassiveEffectsImpl(root, expirationTime) { } if (enableSchedulerTracing) { - __interactionsRef.current = ((prevInteractions: any): Set); + popInteractions(((prevInteractions: any): Set)); finishPendingInteractions(root, expirationTime); } From 30177810239ec53f47dd1b90f438654ca7d1ed66 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 16 Sep 2019 13:46:59 -0700 Subject: [PATCH 4/9] Extract `catch` block into helper function Similar to previous commit. Extract error handling logic into a separate function so it can be reused. --- .../src/ReactFiberWorkLoop.js | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 4ea48c051853..4d49c9700a67 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -1268,8 +1268,7 @@ function renderRoot( resetContextDependencies(); resetHooks(); - const sourceFiber = workInProgress; - if (sourceFiber === null || sourceFiber.return === null) { + if (workInProgress === null || workInProgress.return === null) { // Expected to be working on a non-root fiber. This is a fatal error // because there's no ancestor that can handle it; the root is // supposed to capture all errors that weren't caught by an error @@ -1280,24 +1279,12 @@ function renderRoot( throw thrownValue; } - if (enableProfilerTimer && sourceFiber.mode & ProfileMode) { - // Record the time spent rendering before an error was thrown. This - // avoids inaccurate Profiler durations in the case of a - // suspended render. - stopProfilerTimerIfRunningAndRecordDelta(sourceFiber, true); - } - - const returnFiber = sourceFiber.return; - throwException( + workInProgress = handleError( root, - returnFiber, - sourceFiber, + workInProgress.return, + workInProgress, 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); resetContextDependencies(); @@ -1309,6 +1296,26 @@ function renderRoot( } } +function handleError(root, returnFiber, sourceFiber, thrownValue) { + if (enableProfilerTimer && sourceFiber.mode & ProfileMode) { + // Record the time spent rendering before an error was thrown. This + // avoids inaccurate Profiler durations in the case of a + // suspended render. + stopProfilerTimerIfRunningAndRecordDelta(sourceFiber, true); + } + + throwException( + root, + returnFiber, + sourceFiber, + thrownValue, + renderExpirationTime, + ); + // TODO: This is not wrapped in a try-catch, so if the complete phase + // throws, we won't capture it. + return completeUnitOfWork(sourceFiber); +} + function pushDispatcher(root) { const prevDispatcher = ReactCurrentDispatcher.current; ReactCurrentDispatcher.current = ContextOnlyDispatcher; From 0f64c2deccc11090ab2313c67fb7635a42c6b530 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 16 Sep 2019 13:47:40 -0700 Subject: [PATCH 5/9] Fork `renderRoot` for sync and concurrent Removes `isSync` argument in favor of separate functions. --- .../src/ReactFiberWorkLoop.js | 76 ++++++++++++++++--- 1 file changed, 64 insertions(+), 12 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 4d49c9700a67..358d454108da 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -656,7 +656,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { if (expirationTime !== NoWork) { const originalCallbackNode = root.callbackNode; try { - renderRoot(root, expirationTime, false); + renderRootConcurrent(root, expirationTime); if (workInProgress !== null) { // There's still work left over. Exit without committing. stopInterruptedWorkLoopTimer(); @@ -958,7 +958,7 @@ function performSyncWorkOnRoot(root) { // batch.commit() API. commitRoot(root); } else { - renderRoot(root, expirationTime, true); + renderRootSync(root, expirationTime); invariant( workInProgressRootExitStatus !== RootIncomplete, 'Cannot commit an incomplete root. This error is likely caused by a ' + @@ -1223,12 +1223,9 @@ function prepareFreshStack(root, expirationTime) { } } -// renderRoot should only be called from inside either -// `performConcurrentWorkOnRoot` or `performSyncWorkOnRoot`. -function renderRoot( +function renderRootConcurrent( root: FiberRoot, expirationTime: ExpirationTime, - isSync: boolean, ): void { invariant( (executionContext & (RenderContext | CommitContext)) === NoContext, @@ -1255,13 +1252,68 @@ function renderRoot( do { try { - // TODO: This is now the only place that `isSync` is used. Consider - // outlining the contents of `renderRoot`. - if (isSync) { - workLoopSync(); - } else { - workLoop(); + workLoop(); + break; + } catch (thrownValue) { + // Reset module-level state that was set during the render phase. + resetContextDependencies(); + resetHooks(); + + if (workInProgress === null || workInProgress.return === null) { + // Expected to be working on a non-root fiber. This is a fatal error + // because there's no ancestor that can handle it; the root is + // supposed to capture all errors that weren't caught by an error + // boundary. + prepareFreshStack(root, expirationTime); + executionContext = prevExecutionContext; + markRootSuspendedAtTime(root, expirationTime); + throw thrownValue; } + + workInProgress = handleError( + root, + workInProgress.return, + workInProgress, + thrownValue, + ); + } + } while (true); + resetContextDependencies(); + executionContext = prevExecutionContext; + popDispatcher(prevDispatcher); + if (enableSchedulerTracing) { + popInteractions(((prevInteractions: any): Set)); + } + } +} + +function renderRootSync(root: FiberRoot, expirationTime: ExpirationTime): void { + invariant( + (executionContext & (RenderContext | CommitContext)) === NoContext, + 'Should not already be working.', + ); + + flushPassiveEffects(); + + // If the root or expiration time have changed, throw out the existing stack + // and prepare a fresh one. Otherwise we'll continue where we left off. + if (root !== workInProgressRoot || expirationTime !== renderExpirationTime) { + prepareFreshStack(root, expirationTime); + startWorkOnPendingInteractions(root, expirationTime); + } + + // If we have a work-in-progress fiber, it means there's still work to do + // in this root. + if (workInProgress !== null) { + const prevExecutionContext = executionContext; + executionContext |= RenderContext; + const prevDispatcher = pushDispatcher(root); + const prevInteractions = pushInteractions(root); + startWorkLoopTimer(workInProgress); + + do { + try { + workLoopSync(); break; } catch (thrownValue) { // Reset module-level state that was set during the render phase. From acfb818322d0bd3f8135e446c78fecd73085ff53 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 16 Sep 2019 13:55:13 -0700 Subject: [PATCH 6/9] Extra "root completion" logic to separate function Moving this out to avoid an accidental early return, which would bypass the call to `ensureRootIsScheduled` and freeze the UI. --- .../src/ReactFiberWorkLoop.js | 507 +++++++++--------- 1 file changed, 254 insertions(+), 253 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 358d454108da..49d1281dd49e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -671,278 +671,279 @@ function performConcurrentWorkOnRoot(root, didTimeout) { resolveLocksOnRoot(root, expirationTime); - // Set this to null to indicate there's no in-progress render. - workInProgressRoot = null; + finishConcurrentRender( + root, + finishedWork, + workInProgressRootExitStatus, + expirationTime, + ); + } + // Before exiting, make sure there's a callback scheduled for the + // pending level. This is intentionally duplicated in the `catch` block, + // instead of using `finally`, because it needs to happen before we + // possibly return a continuation, and we can't return in the `finally` + // block without suppressing a potential error. + ensureRootIsScheduled(root); + if (root.callbackNode === originalCallbackNode) { + // The task node scheduled for this root is the same one that's + // currently executed. Need to return a continuation. + return performConcurrentWorkOnRoot.bind(null, root); + } + } catch (error) { + ensureRootIsScheduled(root); + throw error; + } + } + return null; +} - switch (workInProgressRootExitStatus) { - case RootIncomplete: { - invariant(false, 'Should have a work-in-progress.'); - } - // Flow knows about invariant, so it complains if I add a break - // statement, but eslint doesn't know about invariant, so it complains - // if I do. eslint-disable-next-line no-fallthrough - case RootErrored: { - if (expirationTime !== Idle) { - // If this was an async render, the error may have happened due to - // a mutation in a concurrent event. Try rendering one more time, - // synchronously, to see if the error goes away. If there are - // lower priority updates, let's include those, too, in case they - // fix the inconsistency. Render at Idle to include all updates. - markRootExpiredAtTime(root, Idle); - break; - } - // Commit the root in its errored state. - commitRoot(root); - break; - } - case RootSuspended: { - markRootSuspendedAtTime(root, expirationTime); - const lastSuspendedTime = root.lastSuspendedTime; - if (expirationTime === lastSuspendedTime) { - root.nextKnownPendingLevel = getRemainingExpirationTime( - finishedWork, - ); - } - flushSuspensePriorityWarningInDEV(); +function finishConcurrentRender( + root, + finishedWork, + exitStatus, + expirationTime, +) { + // Set this to null to indicate there's no in-progress render. + workInProgressRoot = null; - // We have an acceptable loading state. We need to figure out if we - // should immediately commit it or wait a bit. - - // If we have processed new updates during this render, we may now - // have a new loading state ready. We want to ensure that we commit - // that as soon as possible. - const hasNotProcessedNewUpdates = - workInProgressRootLatestProcessedExpirationTime === Sync; - if ( - hasNotProcessedNewUpdates && - // do not delay if we're inside an act() scope - !( - __DEV__ && - flushSuspenseFallbacksInTests && - IsThisRendererActing.current - ) - ) { - // If we have not processed any new updates during this pass, then - // this is either a retry of an existing fallback state or a - // hidden tree. Hidden trees shouldn't be batched with other work - // and after that's fixed it can only be a retry. We're going to - // throttle committing retries so that we don't show too many - // loading states too quickly. - let msUntilTimeout = - globalMostRecentFallbackTime + FALLBACK_THROTTLE_MS - now(); - // Don't bother with a very short suspense time. - if (msUntilTimeout > 10) { - if (workInProgressRootHasPendingPing) { - const lastPingedTime = root.lastPingedTime; - if ( - lastPingedTime === NoWork || - lastPingedTime >= expirationTime - ) { - // This render was pinged but we didn't get to restart - // earlier so try restarting now instead. - root.lastPingedTime = expirationTime; - prepareFreshStack(root, expirationTime); - break; - } - } + switch (exitStatus) { + case RootIncomplete: { + invariant(false, 'Should have a work-in-progress.'); + } + // Flow knows about invariant, so it complains if I add a break + // statement, but eslint doesn't know about invariant, so it complains + // if I do. eslint-disable-next-line no-fallthrough + case RootErrored: { + if (expirationTime !== Idle) { + // If this was an async render, the error may have happened due to + // a mutation in a concurrent event. Try rendering one more time, + // synchronously, to see if the error goes away. If there are + // lower priority updates, let's include those, too, in case they + // fix the inconsistency. Render at Idle to include all updates. + markRootExpiredAtTime(root, Idle); + break; + } + // Commit the root in its errored state. + commitRoot(root); + break; + } + case RootSuspended: { + markRootSuspendedAtTime(root, expirationTime); + const lastSuspendedTime = root.lastSuspendedTime; + if (expirationTime === lastSuspendedTime) { + root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork); + } + flushSuspensePriorityWarningInDEV(); - const nextTime = getNextRootExpirationTimeToWorkOn(root); - if (nextTime !== NoWork && nextTime !== expirationTime) { - // There's additional work on this root. - break; - } - if ( - lastSuspendedTime !== NoWork && - lastSuspendedTime !== expirationTime - ) { - // We should prefer to render the fallback of at the last - // suspended level. Ping the last suspended level to try - // rendering it again. - root.lastPingedTime = lastSuspendedTime; - break; - } + // We have an acceptable loading state. We need to figure out if we + // should immediately commit it or wait a bit. - // The render is suspended, it hasn't timed out, and there's no - // lower priority work to do. Instead of committing the fallback - // immediately, wait for more data to arrive. - root.timeoutHandle = scheduleTimeout( - commitRoot.bind(null, root), - msUntilTimeout, - ); - break; - } + // If we have processed new updates during this render, we may now + // have a new loading state ready. We want to ensure that we commit + // that as soon as possible. + const hasNotProcessedNewUpdates = + workInProgressRootLatestProcessedExpirationTime === Sync; + if ( + hasNotProcessedNewUpdates && + // do not delay if we're inside an act() scope + !( + __DEV__ && + flushSuspenseFallbacksInTests && + IsThisRendererActing.current + ) + ) { + // If we have not processed any new updates during this pass, then + // this is either a retry of an existing fallback state or a + // hidden tree. Hidden trees shouldn't be batched with other work + // and after that's fixed it can only be a retry. We're going to + // throttle committing retries so that we don't show too many + // loading states too quickly. + let msUntilTimeout = + globalMostRecentFallbackTime + FALLBACK_THROTTLE_MS - now(); + // Don't bother with a very short suspense time. + if (msUntilTimeout > 10) { + if (workInProgressRootHasPendingPing) { + const lastPingedTime = root.lastPingedTime; + if (lastPingedTime === NoWork || lastPingedTime >= expirationTime) { + // This render was pinged but we didn't get to restart + // earlier so try restarting now instead. + root.lastPingedTime = expirationTime; + prepareFreshStack(root, expirationTime); + break; } - // The work expired. Commit immediately. - commitRoot(root); - break; } - case RootSuspendedWithDelay: { - markRootSuspendedAtTime(root, expirationTime); - const lastSuspendedTime = root.lastSuspendedTime; - if (expirationTime === lastSuspendedTime) { - root.nextKnownPendingLevel = getRemainingExpirationTime( - finishedWork, - ); - } - flushSuspensePriorityWarningInDEV(); - - if ( - // do not delay if we're inside an act() scope - !( - __DEV__ && - flushSuspenseFallbacksInTests && - IsThisRendererActing.current - ) - ) { - // We're suspended in a state that should be avoided. We'll try to - // avoid committing it for as long as the timeouts let us. - if (workInProgressRootHasPendingPing) { - const lastPingedTime = root.lastPingedTime; - if ( - lastPingedTime === NoWork || - lastPingedTime >= expirationTime - ) { - // This render was pinged but we didn't get to restart earlier - // so try restarting now instead. - root.lastPingedTime = expirationTime; - prepareFreshStack(root, expirationTime); - break; - } - } - - const nextTime = getNextRootExpirationTimeToWorkOn(root); - if (nextTime !== NoWork && nextTime !== expirationTime) { - // There's additional work on this root. - break; - } - if ( - lastSuspendedTime !== NoWork && - lastSuspendedTime !== expirationTime - ) { - // We should prefer to render the fallback of at the last - // suspended level. Ping the last suspended level to try - // rendering it again. - root.lastPingedTime = lastSuspendedTime; - break; - } - - let msUntilTimeout; - if (workInProgressRootLatestSuspenseTimeout !== Sync) { - // We have processed a suspense config whose expiration time we - // can use as the timeout. - msUntilTimeout = - expirationTimeToMs(workInProgressRootLatestSuspenseTimeout) - - now(); - } else if ( - workInProgressRootLatestProcessedExpirationTime === Sync - ) { - // This should never normally happen because only new updates - // cause delayed states, so we should have processed something. - // However, this could also happen in an offscreen tree. - msUntilTimeout = 0; - } else { - // If we don't have a suspense config, we're going to use a - // heuristic to determine how long we can suspend. - const eventTimeMs: number = inferTimeFromExpirationTime( - workInProgressRootLatestProcessedExpirationTime, - ); - const currentTimeMs = now(); - const timeUntilExpirationMs = - expirationTimeToMs(expirationTime) - currentTimeMs; - let timeElapsed = currentTimeMs - eventTimeMs; - if (timeElapsed < 0) { - // We get this wrong some time since we estimate the time. - timeElapsed = 0; - } - - msUntilTimeout = jnd(timeElapsed) - timeElapsed; - // Clamp the timeout to the expiration time. TODO: Once the - // event time is exact instead of inferred from expiration time - // we don't need this. - if (timeUntilExpirationMs < msUntilTimeout) { - msUntilTimeout = timeUntilExpirationMs; - } - } - - // Don't bother with a very short suspense time. - if (msUntilTimeout > 10) { - // The render is suspended, it hasn't timed out, and there's no - // lower priority work to do. Instead of committing the fallback - // immediately, wait for more data to arrive. - root.timeoutHandle = scheduleTimeout( - commitRoot.bind(null, root), - msUntilTimeout, - ); - break; - } - } - // The work expired. Commit immediately. - commitRoot(root); + const nextTime = getNextRootExpirationTimeToWorkOn(root); + if (nextTime !== NoWork && nextTime !== expirationTime) { + // There's additional work on this root. break; } - case RootCompleted: { - // The work completed. Ready to commit. - if ( - // do not delay if we're inside an act() scope - !( - __DEV__ && - flushSuspenseFallbacksInTests && - IsThisRendererActing.current - ) && - workInProgressRootLatestProcessedExpirationTime !== Sync && - workInProgressRootCanSuspendUsingConfig !== null - ) { - // If we have exceeded the minimum loading delay, which probably - // means we have shown a spinner already, we might have to suspend - // a bit longer to ensure that the spinner is shown for - // enough time. - const msUntilTimeout = computeMsUntilSuspenseLoadingDelay( - workInProgressRootLatestProcessedExpirationTime, - expirationTime, - workInProgressRootCanSuspendUsingConfig, - ); - if (msUntilTimeout > 10) { - markRootSuspendedAtTime(root, expirationTime); - root.timeoutHandle = scheduleTimeout( - commitRoot.bind(null, root), - msUntilTimeout, - ); - break; - } - } - commitRoot(root); + if ( + lastSuspendedTime !== NoWork && + lastSuspendedTime !== expirationTime + ) { + // We should prefer to render the fallback of at the last + // suspended level. Ping the last suspended level to try + // rendering it again. + root.lastPingedTime = lastSuspendedTime; break; } - case RootLocked: { - // This root has a lock that prevents it from committing. Exit. If - // we begin work on the root again, without any intervening updates, - // it will finish without doing additional work. - markRootSuspendedAtTime(root, expirationTime); + + // The render is suspended, it hasn't timed out, and there's no + // lower priority work to do. Instead of committing the fallback + // immediately, wait for more data to arrive. + root.timeoutHandle = scheduleTimeout( + commitRoot.bind(null, root), + msUntilTimeout, + ); + break; + } + } + // The work expired. Commit immediately. + commitRoot(root); + break; + } + case RootSuspendedWithDelay: { + markRootSuspendedAtTime(root, expirationTime); + const lastSuspendedTime = root.lastSuspendedTime; + if (expirationTime === lastSuspendedTime) { + root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork); + } + flushSuspensePriorityWarningInDEV(); + + if ( + // do not delay if we're inside an act() scope + !( + __DEV__ && + flushSuspenseFallbacksInTests && + IsThisRendererActing.current + ) + ) { + // We're suspended in a state that should be avoided. We'll try to + // avoid committing it for as long as the timeouts let us. + if (workInProgressRootHasPendingPing) { + const lastPingedTime = root.lastPingedTime; + if (lastPingedTime === NoWork || lastPingedTime >= expirationTime) { + // This render was pinged but we didn't get to restart earlier + // so try restarting now instead. + root.lastPingedTime = expirationTime; + prepareFreshStack(root, expirationTime); break; } - default: { - invariant(false, 'Unknown root exit status.'); + } + + const nextTime = getNextRootExpirationTimeToWorkOn(root); + if (nextTime !== NoWork && nextTime !== expirationTime) { + // There's additional work on this root. + break; + } + if ( + lastSuspendedTime !== NoWork && + lastSuspendedTime !== expirationTime + ) { + // We should prefer to render the fallback of at the last + // suspended level. Ping the last suspended level to try + // rendering it again. + root.lastPingedTime = lastSuspendedTime; + break; + } + + let msUntilTimeout; + if (workInProgressRootLatestSuspenseTimeout !== Sync) { + // We have processed a suspense config whose expiration time we + // can use as the timeout. + msUntilTimeout = + expirationTimeToMs(workInProgressRootLatestSuspenseTimeout) - now(); + } else if (workInProgressRootLatestProcessedExpirationTime === Sync) { + // This should never normally happen because only new updates + // cause delayed states, so we should have processed something. + // However, this could also happen in an offscreen tree. + msUntilTimeout = 0; + } else { + // If we don't have a suspense config, we're going to use a + // heuristic to determine how long we can suspend. + const eventTimeMs: number = inferTimeFromExpirationTime( + workInProgressRootLatestProcessedExpirationTime, + ); + const currentTimeMs = now(); + const timeUntilExpirationMs = + expirationTimeToMs(expirationTime) - currentTimeMs; + let timeElapsed = currentTimeMs - eventTimeMs; + if (timeElapsed < 0) { + // We get this wrong some time since we estimate the time. + timeElapsed = 0; + } + + msUntilTimeout = jnd(timeElapsed) - timeElapsed; + + // Clamp the timeout to the expiration time. TODO: Once the + // event time is exact instead of inferred from expiration time + // we don't need this. + if (timeUntilExpirationMs < msUntilTimeout) { + msUntilTimeout = timeUntilExpirationMs; } } + + // Don't bother with a very short suspense time. + if (msUntilTimeout > 10) { + // The render is suspended, it hasn't timed out, and there's no + // lower priority work to do. Instead of committing the fallback + // immediately, wait for more data to arrive. + root.timeoutHandle = scheduleTimeout( + commitRoot.bind(null, root), + msUntilTimeout, + ); + break; + } } - // Before exiting, make sure there's a callback scheduled for the - // pending level. This is intentionally duplicated in the `catch` block, - // instead of using `finally`, because it needs to happen before we - // possibly return a continuation, and we can't return in the `finally` - // block without suppressing a potential error. - ensureRootIsScheduled(root); - if (root.callbackNode === originalCallbackNode) { - // The task node scheduled for this root is the same one that's - // currently executed. Need to return a continuation. - return performConcurrentWorkOnRoot.bind(null, root); + // The work expired. Commit immediately. + commitRoot(root); + break; + } + case RootCompleted: { + // The work completed. Ready to commit. + if ( + // do not delay if we're inside an act() scope + !( + __DEV__ && + flushSuspenseFallbacksInTests && + IsThisRendererActing.current + ) && + workInProgressRootLatestProcessedExpirationTime !== Sync && + workInProgressRootCanSuspendUsingConfig !== null + ) { + // If we have exceeded the minimum loading delay, which probably + // means we have shown a spinner already, we might have to suspend + // a bit longer to ensure that the spinner is shown for + // enough time. + const msUntilTimeout = computeMsUntilSuspenseLoadingDelay( + workInProgressRootLatestProcessedExpirationTime, + expirationTime, + workInProgressRootCanSuspendUsingConfig, + ); + if (msUntilTimeout > 10) { + markRootSuspendedAtTime(root, expirationTime); + root.timeoutHandle = scheduleTimeout( + commitRoot.bind(null, root), + msUntilTimeout, + ); + break; + } } - } catch (error) { - ensureRootIsScheduled(root); - throw error; + commitRoot(root); + break; + } + case RootLocked: { + // This root has a lock that prevents it from committing. Exit. If + // we begin work on the root again, without any intervening updates, + // it will finish without doing additional work. + markRootSuspendedAtTime(root, expirationTime); + break; + } + default: { + invariant(false, 'Unknown root exit status.'); } } - return null; } // This is the entry point for synchronous tasks that don't go From 35ab8c03ef8e80060c580109cdc92ae8b8e69ee4 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 16 Sep 2019 14:49:06 -0700 Subject: [PATCH 7/9] Inline `renderRoot` Inlines `renderRoot` into `performConcurrentWorkOnRoot` and `performSyncWorkOnRoot`. This lets me remove the `isSync` argument and also get rid of a redundant try-catch wrapper. --- .../src/ReactFiberWorkLoop.js | 381 +++++++++--------- 1 file changed, 184 insertions(+), 197 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 49d1281dd49e..23e2ab873dbd 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -655,43 +655,96 @@ function performConcurrentWorkOnRoot(root, didTimeout) { const expirationTime = getNextRootExpirationTimeToWorkOn(root); if (expirationTime !== NoWork) { const originalCallbackNode = root.callbackNode; - try { - renderRootConcurrent(root, expirationTime); - if (workInProgress !== null) { - // There's still work left over. Exit without committing. - stopInterruptedWorkLoopTimer(); - } else { - // We now have a consistent tree. The next step is either to commit it, - // or, if something suspended, wait to commit it after a timeout. - stopFinishedWorkLoopTimer(); + invariant( + (executionContext & (RenderContext | CommitContext)) === NoContext, + 'Should not already be working.', + ); - const finishedWork: Fiber = ((root.finishedWork = - root.current.alternate): any); - root.finishedExpirationTime = expirationTime; + flushPassiveEffects(); - resolveLocksOnRoot(root, expirationTime); + // If the root or expiration time have changed, throw out the existing stack + // and prepare a fresh one. Otherwise we'll continue where we left off. + if ( + root !== workInProgressRoot || + expirationTime !== renderExpirationTime + ) { + prepareFreshStack(root, expirationTime); + startWorkOnPendingInteractions(root, expirationTime); + } + + // If we have a work-in-progress fiber, it means there's still work to do + // in this root. + if (workInProgress !== null) { + const prevExecutionContext = executionContext; + executionContext |= RenderContext; + const prevDispatcher = pushDispatcher(root); + const prevInteractions = pushInteractions(root); + startWorkLoopTimer(workInProgress); + do { + try { + workLoopConcurrent(); + break; + } catch (thrownValue) { + // Reset module-level state that was set during the render phase. + resetContextDependencies(); + resetHooks(); + + if (workInProgress === null || workInProgress.return === null) { + // Expected to be working on a non-root fiber. This is a fatal error + // because there's no ancestor that can handle it; the root is + // supposed to capture all errors that weren't caught by an error + // boundary. + prepareFreshStack(root, expirationTime); + executionContext = prevExecutionContext; + markRootSuspendedAtTime(root, expirationTime); + ensureRootIsScheduled(root); + throw thrownValue; + } - finishConcurrentRender( - root, - finishedWork, - workInProgressRootExitStatus, - expirationTime, - ); - } - // Before exiting, make sure there's a callback scheduled for the - // pending level. This is intentionally duplicated in the `catch` block, - // instead of using `finally`, because it needs to happen before we - // possibly return a continuation, and we can't return in the `finally` - // block without suppressing a potential error. - ensureRootIsScheduled(root); - if (root.callbackNode === originalCallbackNode) { - // The task node scheduled for this root is the same one that's - // currently executed. Need to return a continuation. - return performConcurrentWorkOnRoot.bind(null, root); + workInProgress = handleError( + root, + workInProgress.return, + workInProgress, + thrownValue, + ); + } + } while (true); + resetContextDependencies(); + executionContext = prevExecutionContext; + popDispatcher(prevDispatcher); + if (enableSchedulerTracing) { + popInteractions(((prevInteractions: any): Set)); } - } catch (error) { - ensureRootIsScheduled(root); - throw error; + } + + if (workInProgress !== null) { + // There's still work left over. Exit without committing. + stopInterruptedWorkLoopTimer(); + } else { + // We now have a consistent tree. The next step is either to commit it, + // or, if something suspended, wait to commit it after a timeout. + stopFinishedWorkLoopTimer(); + + const finishedWork: Fiber = ((root.finishedWork = + root.current.alternate): any); + root.finishedExpirationTime = expirationTime; + + resolveLocksOnRoot(root, expirationTime); + + finishConcurrentRender( + root, + finishedWork, + workInProgressRootExitStatus, + expirationTime, + ); + } + // Before exiting, make sure there's a callback scheduled for the next + // pending level. + ensureRootIsScheduled(root); + if (root.callbackNode === originalCallbackNode) { + // The task node scheduled for this root is the same one that's + // currently executed. Need to return a continuation. + return performConcurrentWorkOnRoot.bind(null, root); } } return null; @@ -952,53 +1005,112 @@ function performSyncWorkOnRoot(root) { // Check if there's expired work on this root. Otherwise, render at Sync. const lastExpiredTime = root.lastExpiredTime; const expirationTime = lastExpiredTime !== NoWork ? lastExpiredTime : Sync; - try { - if (root.finishedExpirationTime === expirationTime) { - // There's already a pending commit at this expiration time. - // TODO: This is poorly factored. This case only exists for the - // batch.commit() API. - commitRoot(root); - } else { - renderRootSync(root, expirationTime); - invariant( - workInProgressRootExitStatus !== RootIncomplete, - 'Cannot commit an incomplete root. This error is likely caused by a ' + - 'bug in React. Please file an issue.', - ); + if (root.finishedExpirationTime === expirationTime) { + // There's already a pending commit at this expiration time. + // TODO: This is poorly factored. This case only exists for the + // batch.commit() API. + commitRoot(root); + } else { + invariant( + (executionContext & (RenderContext | CommitContext)) === NoContext, + 'Should not already be working.', + ); - // We now have a consistent tree. The next step is either to commit it, - // or, if something suspended, wait to commit it after a timeout. - stopFinishedWorkLoopTimer(); + flushPassiveEffects(); - root.finishedWork = ((root.current.alternate: any): Fiber); - root.finishedExpirationTime = expirationTime; + // If the root or expiration time have changed, throw out the existing stack + // and prepare a fresh one. Otherwise we'll continue where we left off. + if ( + root !== workInProgressRoot || + expirationTime !== renderExpirationTime + ) { + prepareFreshStack(root, expirationTime); + startWorkOnPendingInteractions(root, expirationTime); + } - resolveLocksOnRoot(root, expirationTime); - if (workInProgressRootExitStatus === RootLocked) { - // This root has a lock that prevents it from committing. Exit. If we - // begin work on the root again, without any intervening updates, it - // will finish without doing additional work. - markRootSuspendedAtTime(root, expirationTime); - } else { - // Set this to null to indicate there's no in-progress render. - workInProgressRoot = null; + // If we have a work-in-progress fiber, it means there's still work to do + // in this root. + if (workInProgress !== null) { + const prevExecutionContext = executionContext; + executionContext |= RenderContext; + const prevDispatcher = pushDispatcher(root); + const prevInteractions = pushInteractions(root); + startWorkLoopTimer(workInProgress); - if (__DEV__) { - if ( - workInProgressRootExitStatus === RootSuspended || - workInProgressRootExitStatus === RootSuspendedWithDelay - ) { - flushSuspensePriorityWarningInDEV(); + do { + try { + workLoopSync(); + break; + } catch (thrownValue) { + // Reset module-level state that was set during the render phase. + resetContextDependencies(); + resetHooks(); + + if (workInProgress === null || workInProgress.return === null) { + // Expected to be working on a non-root fiber. This is a fatal error + // because there's no ancestor that can handle it; the root is + // supposed to capture all errors that weren't caught by an error + // boundary. + prepareFreshStack(root, expirationTime); + executionContext = prevExecutionContext; + markRootSuspendedAtTime(root, expirationTime); + ensureRootIsScheduled(root); + throw thrownValue; } + + workInProgress = handleError( + root, + workInProgress.return, + workInProgress, + thrownValue, + ); } - commitRoot(root); + } while (true); + resetContextDependencies(); + executionContext = prevExecutionContext; + popDispatcher(prevDispatcher); + if (enableSchedulerTracing) { + popInteractions(((prevInteractions: any): Set)); } } - } finally { - // Before exiting, make sure there's a callback scheduled for the - // pending level. - ensureRootIsScheduled(root); + + invariant( + workInProgressRootExitStatus !== RootIncomplete, + 'Cannot commit an incomplete root. This error is likely caused by a ' + + 'bug in React. Please file an issue.', + ); + + // We now have a consistent tree. The next step is either to commit it, + // or, if something suspended, wait to commit it after a timeout. + stopFinishedWorkLoopTimer(); + + root.finishedWork = ((root.current.alternate: any): Fiber); + root.finishedExpirationTime = expirationTime; + + resolveLocksOnRoot(root, expirationTime); + if (workInProgressRootExitStatus === RootLocked) { + // This root has a lock that prevents it from committing. Exit. If we + // begin work on the root again, without any intervening updates, it + // will finish without doing additional work. + markRootSuspendedAtTime(root, expirationTime); + } else { + // Set this to null to indicate there's no in-progress render. + workInProgressRoot = null; + + if (__DEV__) { + if ( + workInProgressRootExitStatus === RootSuspended || + workInProgressRootExitStatus === RootSuspendedWithDelay + ) { + flushSuspensePriorityWarningInDEV(); + } + } + commitRoot(root); + } } + // Before exiting, make sure there's a callback scheduled for the next + // pending level. + ensureRootIsScheduled(root); return null; } @@ -1224,131 +1336,6 @@ function prepareFreshStack(root, expirationTime) { } } -function renderRootConcurrent( - root: FiberRoot, - expirationTime: ExpirationTime, -): void { - invariant( - (executionContext & (RenderContext | CommitContext)) === NoContext, - 'Should not already be working.', - ); - - flushPassiveEffects(); - - // If the root or expiration time have changed, throw out the existing stack - // and prepare a fresh one. Otherwise we'll continue where we left off. - if (root !== workInProgressRoot || expirationTime !== renderExpirationTime) { - prepareFreshStack(root, expirationTime); - startWorkOnPendingInteractions(root, expirationTime); - } - - // If we have a work-in-progress fiber, it means there's still work to do - // in this root. - if (workInProgress !== null) { - const prevExecutionContext = executionContext; - executionContext |= RenderContext; - const prevDispatcher = pushDispatcher(root); - const prevInteractions = pushInteractions(root); - startWorkLoopTimer(workInProgress); - - do { - try { - workLoop(); - break; - } catch (thrownValue) { - // Reset module-level state that was set during the render phase. - resetContextDependencies(); - resetHooks(); - - if (workInProgress === null || workInProgress.return === null) { - // Expected to be working on a non-root fiber. This is a fatal error - // because there's no ancestor that can handle it; the root is - // supposed to capture all errors that weren't caught by an error - // boundary. - prepareFreshStack(root, expirationTime); - executionContext = prevExecutionContext; - markRootSuspendedAtTime(root, expirationTime); - throw thrownValue; - } - - workInProgress = handleError( - root, - workInProgress.return, - workInProgress, - thrownValue, - ); - } - } while (true); - resetContextDependencies(); - executionContext = prevExecutionContext; - popDispatcher(prevDispatcher); - if (enableSchedulerTracing) { - popInteractions(((prevInteractions: any): Set)); - } - } -} - -function renderRootSync(root: FiberRoot, expirationTime: ExpirationTime): void { - invariant( - (executionContext & (RenderContext | CommitContext)) === NoContext, - 'Should not already be working.', - ); - - flushPassiveEffects(); - - // If the root or expiration time have changed, throw out the existing stack - // and prepare a fresh one. Otherwise we'll continue where we left off. - if (root !== workInProgressRoot || expirationTime !== renderExpirationTime) { - prepareFreshStack(root, expirationTime); - startWorkOnPendingInteractions(root, expirationTime); - } - - // If we have a work-in-progress fiber, it means there's still work to do - // in this root. - if (workInProgress !== null) { - const prevExecutionContext = executionContext; - executionContext |= RenderContext; - const prevDispatcher = pushDispatcher(root); - const prevInteractions = pushInteractions(root); - startWorkLoopTimer(workInProgress); - - do { - try { - workLoopSync(); - break; - } catch (thrownValue) { - // Reset module-level state that was set during the render phase. - resetContextDependencies(); - resetHooks(); - - if (workInProgress === null || workInProgress.return === null) { - // Expected to be working on a non-root fiber. This is a fatal error - // because there's no ancestor that can handle it; the root is - // supposed to capture all errors that weren't caught by an error - // boundary. - prepareFreshStack(root, expirationTime); - executionContext = prevExecutionContext; - markRootSuspendedAtTime(root, expirationTime); - throw thrownValue; - } - - workInProgress = handleError( - root, - workInProgress.return, - workInProgress, - thrownValue, - ); - } - } while (true); - resetContextDependencies(); - executionContext = prevExecutionContext; - popDispatcher(prevDispatcher); - if (enableSchedulerTracing) { - popInteractions(((prevInteractions: any): Set)); - } - } -} - function handleError(root, returnFiber, sourceFiber, thrownValue) { if (enableProfilerTimer && sourceFiber.mode & ProfileMode) { // Record the time spent rendering before an error was thrown. This @@ -1511,7 +1498,7 @@ function workLoopSync() { } /** @noinline */ -function workLoop() { +function workLoopConcurrent() { // Perform work until Scheduler asks us to yield while (workInProgress !== null && !shouldYield()) { workInProgress = performUnitOfWork(workInProgress); From 17d59fc25a7240d2348d4fe4ee7e3693cc8c81ed Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 16 Sep 2019 15:55:58 -0700 Subject: [PATCH 8/9] Remove ad hoc `throw` Fatal errors (errors that are not captured by an error boundary) are currently rethrown from directly inside the render phase's `catch` block. This is a refactor hazard because the code in this branch has to mirror the code that happens at the end of the function, when exiting the render phase in the normal case. This commit moves the throw to the end, using a new root exit status. --- .../src/ReactFiberWorkLoop.js | 228 +++++++++--------- scripts/error-codes/codes.json | 3 +- 2 files changed, 116 insertions(+), 115 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 23e2ab873dbd..3184f66204f8 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -200,13 +200,14 @@ const LegacyUnbatchedContext = /* */ 0b001000; const RenderContext = /* */ 0b010000; const CommitContext = /* */ 0b100000; -type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5; +type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5 | 6; const RootIncomplete = 0; -const RootErrored = 1; -const RootSuspended = 2; -const RootSuspendedWithDelay = 3; -const RootCompleted = 4; -const RootLocked = 5; +const RootFatalErrored = 1; +const RootErrored = 2; +const RootSuspended = 3; +const RootSuspendedWithDelay = 4; +const RootCompleted = 5; +const RootLocked = 6; export type Thenable = { then(resolve: () => mixed, reject?: () => mixed): Thenable | void, @@ -225,6 +226,8 @@ let workInProgress: Fiber | null = null; let renderExpirationTime: ExpirationTime = NoWork; // Whether to root completed, errored, suspended, etc. let workInProgressRootExitStatus: RootExitStatus = RootIncomplete; +// A fatal error, if one is thrown +let workInProgressRootFatalError: mixed = null; // Most recent event time among processed updates during this render. // This is conceptually a time stamp but expressed in terms of an ExpirationTime // because we deal mostly with expiration times in the hot path, so this avoids @@ -685,28 +688,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { workLoopConcurrent(); break; } catch (thrownValue) { - // Reset module-level state that was set during the render phase. - resetContextDependencies(); - resetHooks(); - - if (workInProgress === null || workInProgress.return === null) { - // Expected to be working on a non-root fiber. This is a fatal error - // because there's no ancestor that can handle it; the root is - // supposed to capture all errors that weren't caught by an error - // boundary. - prepareFreshStack(root, expirationTime); - executionContext = prevExecutionContext; - markRootSuspendedAtTime(root, expirationTime); - ensureRootIsScheduled(root); - throw thrownValue; - } - - workInProgress = handleError( - root, - workInProgress.return, - workInProgress, - thrownValue, - ); + handleError(root, workInProgress, thrownValue); } } while (true); resetContextDependencies(); @@ -715,36 +697,42 @@ function performConcurrentWorkOnRoot(root, didTimeout) { if (enableSchedulerTracing) { popInteractions(((prevInteractions: any): Set)); } - } - - if (workInProgress !== null) { - // There's still work left over. Exit without committing. - stopInterruptedWorkLoopTimer(); - } else { - // We now have a consistent tree. The next step is either to commit it, - // or, if something suspended, wait to commit it after a timeout. - stopFinishedWorkLoopTimer(); - const finishedWork: Fiber = ((root.finishedWork = - root.current.alternate): any); - root.finishedExpirationTime = expirationTime; + if (workInProgressRootExitStatus === RootFatalErrored) { + const fatalError = workInProgressRootFatalError; + stopInterruptedWorkLoopTimer(); + prepareFreshStack(root, expirationTime); + markRootSuspendedAtTime(root, expirationTime); + ensureRootIsScheduled(root); + throw fatalError; + } - resolveLocksOnRoot(root, expirationTime); + if (workInProgress !== null) { + // There's still work left over. Exit without committing. + stopInterruptedWorkLoopTimer(); + } else { + // We now have a consistent tree. The next step is either to commit it, + // or, if something suspended, wait to commit it after a timeout. + stopFinishedWorkLoopTimer(); + + const finishedWork: Fiber = ((root.finishedWork = + root.current.alternate): any); + root.finishedExpirationTime = expirationTime; + resolveLocksOnRoot(root, expirationTime); + finishConcurrentRender( + root, + finishedWork, + workInProgressRootExitStatus, + expirationTime, + ); + } - finishConcurrentRender( - root, - finishedWork, - workInProgressRootExitStatus, - expirationTime, - ); - } - // Before exiting, make sure there's a callback scheduled for the next - // pending level. - ensureRootIsScheduled(root); - if (root.callbackNode === originalCallbackNode) { - // The task node scheduled for this root is the same one that's - // currently executed. Need to return a continuation. - return performConcurrentWorkOnRoot.bind(null, root); + ensureRootIsScheduled(root); + if (root.callbackNode === originalCallbackNode) { + // The task node scheduled for this root is the same one that's + // currently executed. Need to return a continuation. + return performConcurrentWorkOnRoot.bind(null, root); + } } } return null; @@ -760,8 +748,9 @@ function finishConcurrentRender( workInProgressRoot = null; switch (exitStatus) { - case RootIncomplete: { - invariant(false, 'Should have a work-in-progress.'); + case RootIncomplete: + case RootFatalErrored: { + invariant(false, 'Root did not complete. This is a bug in React.'); } // Flow knows about invariant, so it complains if I add a break // statement, but eslint doesn't know about invariant, so it complains @@ -1042,28 +1031,7 @@ function performSyncWorkOnRoot(root) { workLoopSync(); break; } catch (thrownValue) { - // Reset module-level state that was set during the render phase. - resetContextDependencies(); - resetHooks(); - - if (workInProgress === null || workInProgress.return === null) { - // Expected to be working on a non-root fiber. This is a fatal error - // because there's no ancestor that can handle it; the root is - // supposed to capture all errors that weren't caught by an error - // boundary. - prepareFreshStack(root, expirationTime); - executionContext = prevExecutionContext; - markRootSuspendedAtTime(root, expirationTime); - ensureRootIsScheduled(root); - throw thrownValue; - } - - workInProgress = handleError( - root, - workInProgress.return, - workInProgress, - thrownValue, - ); + handleError(root, workInProgress, thrownValue); } } while (true); resetContextDependencies(); @@ -1072,46 +1040,63 @@ function performSyncWorkOnRoot(root) { if (enableSchedulerTracing) { popInteractions(((prevInteractions: any): Set)); } - } - invariant( - workInProgressRootExitStatus !== RootIncomplete, - 'Cannot commit an incomplete root. This error is likely caused by a ' + - 'bug in React. Please file an issue.', - ); + if (workInProgressRootExitStatus === RootFatalErrored) { + const fatalError = workInProgressRootFatalError; + stopInterruptedWorkLoopTimer(); + prepareFreshStack(root, expirationTime); + markRootSuspendedAtTime(root, expirationTime); + ensureRootIsScheduled(root); + throw fatalError; + } - // We now have a consistent tree. The next step is either to commit it, - // or, if something suspended, wait to commit it after a timeout. - stopFinishedWorkLoopTimer(); + if (workInProgress !== null) { + // This is a sync render, so we should have finished the whole tree. + invariant( + false, + 'Cannot commit an incomplete root. This error is likely caused by a ' + + 'bug in React. Please file an issue.', + ); + } else { + // We now have a consistent tree. Because this is a sync render, we + // will commit it even if something suspended. The only exception is + // if the root is locked (using the unstable_createBatch API). + stopFinishedWorkLoopTimer(); + root.finishedWork = (root.current.alternate: any); + root.finishedExpirationTime = expirationTime; + resolveLocksOnRoot(root, expirationTime); + finishSyncRender(root, workInProgressRootExitStatus, expirationTime); + } - root.finishedWork = ((root.current.alternate: any): Fiber); - root.finishedExpirationTime = expirationTime; + // Before exiting, make sure there's a callback scheduled for the next + // pending level. + ensureRootIsScheduled(root); + } + } - resolveLocksOnRoot(root, expirationTime); - if (workInProgressRootExitStatus === RootLocked) { - // This root has a lock that prevents it from committing. Exit. If we - // begin work on the root again, without any intervening updates, it - // will finish without doing additional work. - markRootSuspendedAtTime(root, expirationTime); - } else { - // Set this to null to indicate there's no in-progress render. - workInProgressRoot = null; + return null; +} - if (__DEV__) { - if ( - workInProgressRootExitStatus === RootSuspended || - workInProgressRootExitStatus === RootSuspendedWithDelay - ) { - flushSuspensePriorityWarningInDEV(); - } +function finishSyncRender(root, exitStatus, expirationTime) { + if (exitStatus === RootLocked) { + // This root has a lock that prevents it from committing. Exit. If we + // begin work on the root again, without any intervening updates, it + // will finish without doing additional work. + markRootSuspendedAtTime(root, expirationTime); + } else { + // Set this to null to indicate there's no in-progress render. + workInProgressRoot = null; + + if (__DEV__) { + if ( + exitStatus === RootSuspended || + exitStatus === RootSuspendedWithDelay + ) { + flushSuspensePriorityWarningInDEV(); } - commitRoot(root); } + commitRoot(root); } - // Before exiting, make sure there's a callback scheduled for the next - // pending level. - ensureRootIsScheduled(root); - return null; } export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) { @@ -1320,6 +1305,7 @@ function prepareFreshStack(root, expirationTime) { workInProgress = createWorkInProgress(root.current, null, expirationTime); renderExpirationTime = expirationTime; workInProgressRootExitStatus = RootIncomplete; + workInProgressRootFatalError = null; workInProgressRootLatestProcessedExpirationTime = Sync; workInProgressRootLatestSuspenseTimeout = Sync; workInProgressRootCanSuspendUsingConfig = null; @@ -1336,7 +1322,21 @@ function prepareFreshStack(root, expirationTime) { } } -function handleError(root, returnFiber, sourceFiber, thrownValue) { +function handleError(root, sourceFiber, thrownValue) { + // Reset module-level state that was set during the render phase. + resetContextDependencies(); + resetHooks(); + + if (workInProgress === null || workInProgress.return === null) { + // Expected to be working on a non-root fiber. This is a fatal error + // because there's no ancestor that can handle it; the root is + // supposed to capture all errors that weren't caught by an error + // boundary. + workInProgressRootExitStatus = RootFatalErrored; + workInProgressRootFatalError = thrownValue; + return null; + } + if (enableProfilerTimer && sourceFiber.mode & ProfileMode) { // Record the time spent rendering before an error was thrown. This // avoids inaccurate Profiler durations in the case of a @@ -1346,14 +1346,14 @@ function handleError(root, returnFiber, sourceFiber, thrownValue) { throwException( root, - returnFiber, + workInProgress.return, sourceFiber, thrownValue, renderExpirationTime, ); // TODO: This is not wrapped in a try-catch, so if the complete phase // throws, we won't capture it. - return completeUnitOfWork(sourceFiber); + workInProgress = completeUnitOfWork(sourceFiber); } function pushDispatcher(root) { diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 5cac8a18d89b..00edb16006fa 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -342,5 +342,6 @@ "341": "We just came from a parent so we must have had a parent. This is a bug in React.", "342": "A React component suspended while rendering, but no fallback UI was specified.\n\nAdd a component higher in the tree to provide a loading indicator or placeholder to display.", "343": "ReactDOMServer does not yet support scope components.", - "344": "Expected prepareToHydrateHostSuspenseInstance() to never be called. This error is likely caused by a bug in React. Please file an issue." + "344": "Expected prepareToHydrateHostSuspenseInstance() to never be called. This error is likely caused by a bug in React. Please file an issue.", + "345": "Root did not complete. This is a bug in React." } From d69643c1e69fc7f6a496971126f13094066c499d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 16 Sep 2019 16:33:54 -0700 Subject: [PATCH 9/9] Handle errors that occur on unwind --- .../src/ReactFiberWorkLoop.js | 70 +++++++++++-------- 1 file changed, 39 insertions(+), 31 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 3184f66204f8..50b3d2aaefac 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -688,7 +688,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { workLoopConcurrent(); break; } catch (thrownValue) { - handleError(root, workInProgress, thrownValue); + handleError(root, thrownValue); } } while (true); resetContextDependencies(); @@ -1031,7 +1031,7 @@ function performSyncWorkOnRoot(root) { workLoopSync(); break; } catch (thrownValue) { - handleError(root, workInProgress, thrownValue); + handleError(root, thrownValue); } } while (true); resetContextDependencies(); @@ -1322,38 +1322,46 @@ function prepareFreshStack(root, expirationTime) { } } -function handleError(root, sourceFiber, thrownValue) { - // Reset module-level state that was set during the render phase. - resetContextDependencies(); - resetHooks(); +function handleError(root, thrownValue) { + do { + try { + // Reset module-level state that was set during the render phase. + resetContextDependencies(); + resetHooks(); - if (workInProgress === null || workInProgress.return === null) { - // Expected to be working on a non-root fiber. This is a fatal error - // because there's no ancestor that can handle it; the root is - // supposed to capture all errors that weren't caught by an error - // boundary. - workInProgressRootExitStatus = RootFatalErrored; - workInProgressRootFatalError = thrownValue; - return null; - } + if (workInProgress === null || workInProgress.return === null) { + // Expected to be working on a non-root fiber. This is a fatal error + // because there's no ancestor that can handle it; the root is + // supposed to capture all errors that weren't caught by an error + // boundary. + workInProgressRootExitStatus = RootFatalErrored; + workInProgressRootFatalError = thrownValue; + return null; + } - if (enableProfilerTimer && sourceFiber.mode & ProfileMode) { - // Record the time spent rendering before an error was thrown. This - // avoids inaccurate Profiler durations in the case of a - // suspended render. - stopProfilerTimerIfRunningAndRecordDelta(sourceFiber, true); - } + if (enableProfilerTimer && workInProgress.mode & ProfileMode) { + // Record the time spent rendering before an error was thrown. This + // avoids inaccurate Profiler durations in the case of a + // suspended render. + stopProfilerTimerIfRunningAndRecordDelta(workInProgress, true); + } - throwException( - root, - workInProgress.return, - sourceFiber, - 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); + throwException( + root, + workInProgress.return, + workInProgress, + thrownValue, + renderExpirationTime, + ); + workInProgress = completeUnitOfWork(workInProgress); + } catch (yetAnotherThrownValue) { + // Something in the return path also threw. + thrownValue = yetAnotherThrownValue; + continue; + } + // Return to the normal work loop. + return; + } while (true); } function pushDispatcher(root) {