Skip to content

Commit

Permalink
Don't log recoverable errors until commit phase
Browse files Browse the repository at this point in the history 
If the render is interrupted and restarts, we don't want to log the
errors multiple times.

This change only affects errors that are recovered by de-opting to
synchronous rendering; we'll have to do something else for errors
during hydration, since they use a different recovery path.
  • Loading branch information
@acdlite
acdlite committed Feb 3, 2022
1 parent c6f65a2 commit 5f37862
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 36 deletions.
60 changes: 42 additions & 18 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
import type {StackCursor} from './ReactFiberStack.new';
import type {Flags} from './ReactFiberFlags';
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new';
import type {EventPriority} from './ReactEventPriorities.new';

import {
warnAboutDeprecatedLifecycles,
Expand Down Expand Up @@ -297,7 +298,11 @@ let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
// Lanes that were pinged (in an interleaved event) during this render.
let workInProgressRootPingedLanes: Lanes = NoLanes;
// Errors that are thrown during the render phase.
let workInProgressRootConcurrentErrors: Array<mixed> | null = null;
// These are errors that we recovered from without surfacing them to the UI.
// We will log them once the tree commits.
let workInProgressRootRecoverableErrors: Array<mixed> | null = null;

// The most recent time we committed a fallback. This lets us ensure a train
// model where we don't commit new loading states in too quick succession.
Expand Down Expand Up @@ -896,16 +901,21 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
}
}

const errorsFromFirstAttempt = workInProgressRootConcurrentErrors;
const exitStatus = renderRootSync(root, errorRetryLanes);
const recoverableErrors = workInProgressRootConcurrentErrors;
if (exitStatus !== RootErrored) {
// Successfully finished rendering
if (recoverableErrors !== null) {
// Although we recovered the UI without surfacing an error, we should
// still log the errors so they can be fixed.
for (let j = 0; j < recoverableErrors.length; j++) {
const recoverableError = recoverableErrors[j];
logRecoverableError(root.errorLoggingConfig, recoverableError);
// Successfully finished rendering on retry
if (errorsFromFirstAttempt !== null) {
// The errors from the failed first attempt have been recovered. Add
// them to the collection of recoverable errors. We'll log them in the
// commit phase.
if (workInProgressRootConcurrentErrors === null) {
workInProgressRootRecoverableErrors = errorsFromFirstAttempt;
} else {
workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply(
null,
errorsFromFirstAttempt,
);
}
}
} else {
Expand All @@ -929,7 +939,7 @@ function finishConcurrentRender(root, exitStatus, lanes) {
case RootErrored: {
// We should have already attempted to retry this tree. If we reached
// this point, it errored again. Commit it.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootSuspended: {
Expand Down Expand Up @@ -969,14 +979,14 @@ function finishConcurrentRender(root, exitStatus, lanes) {
// lower priority work to do. Instead of committing the fallback
// immediately, wait for more data to arrive.
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
msUntilTimeout,
);
break;
}
}
// The work expired. Commit immediately.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootSuspendedWithDelay: {
Expand Down Expand Up @@ -1007,20 +1017,20 @@ function finishConcurrentRender(root, exitStatus, lanes) {
// Instead of committing the fallback immediately, wait for more data
// to arrive.
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
msUntilTimeout,
);
break;
}
}

// Commit the placeholder.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootCompleted: {
// The work completed. Ready to commit.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
default: {
Expand Down Expand Up @@ -1140,7 +1150,7 @@ function performSyncWorkOnRoot(root) {
const finishedWork: Fiber = (root.current.alternate: any);
root.finishedWork = finishedWork;
root.finishedLanes = lanes;
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);

// Before exiting, make sure there's a callback scheduled for the next
// pending level.
Expand Down Expand Up @@ -1337,6 +1347,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
workInProgressRootPingedLanes = NoLanes;
workInProgressRootConcurrentErrors = null;
workInProgressRootRecoverableErrors = null;

enqueueInterleavedUpdates();

Expand Down Expand Up @@ -1803,15 +1814,15 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
}
}

function commitRoot(root) {
function commitRoot(root: FiberRoot, recoverableErrors: null | Array<mixed>) {
// TODO: This no longer makes any sense. We already wrap the mutation and
// layout phases. Should be able to remove.
const previousUpdateLanePriority = getCurrentUpdatePriority();
const prevTransition = ReactCurrentBatchConfig.transition;
try {
ReactCurrentBatchConfig.transition = 0;
setCurrentUpdatePriority(DiscreteEventPriority);
commitRootImpl(root, previousUpdateLanePriority);
commitRootImpl(root, recoverableErrors, previousUpdateLanePriority);
} finally {
ReactCurrentBatchConfig.transition = prevTransition;
setCurrentUpdatePriority(previousUpdateLanePriority);
Expand All @@ -1820,7 +1831,11 @@ function commitRoot(root) {
return null;
}

function commitRootImpl(root, renderPriorityLevel) {
function commitRootImpl(
root: FiberRoot,
recoverableErrors: null | Array<mixed>,
renderPriorityLevel: EventPriority,
) {
do {
// `flushPassiveEffects` will call `flushSyncUpdateQueue` at the end, which
// means `flushPassiveEffects` will sometimes result in additional
Expand Down Expand Up @@ -2091,6 +2106,15 @@ function commitRootImpl(root, renderPriorityLevel) {
// additional work on this root is scheduled.
ensureRootIsScheduled(root, now());

if (recoverableErrors !== null) {
// There were errors during this render, but recovered from them without
// needing to surface it to the UI. We log them here.
for (let i = 0; i < recoverableErrors.length; i++) {
const recoverableError = recoverableErrors[i];
logRecoverableError(root.errorLoggingConfig, recoverableError);
}
}

if (hasUncaughtError) {
hasUncaughtError = false;
const error = firstUncaughtError;
Expand Down
60 changes: 42 additions & 18 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {SuspenseState} from './ReactFiberSuspenseComponent.old';
import type {StackCursor} from './ReactFiberStack.old';
import type {Flags} from './ReactFiberFlags';
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.old';
import type {EventPriority} from './ReactEventPriorities.old';

import {
warnAboutDeprecatedLifecycles,
Expand Down Expand Up @@ -297,7 +298,11 @@ let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
// Lanes that were pinged (in an interleaved event) during this render.
let workInProgressRootPingedLanes: Lanes = NoLanes;
// Errors that are thrown during the render phase.
let workInProgressRootConcurrentErrors: Array<mixed> | null = null;
// These are errors that we recovered from without surfacing them to the UI.
// We will log them once the tree commits.
let workInProgressRootRecoverableErrors: Array<mixed> | null = null;

// The most recent time we committed a fallback. This lets us ensure a train
// model where we don't commit new loading states in too quick succession.
Expand Down Expand Up @@ -896,16 +901,21 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
}
}

const errorsFromFirstAttempt = workInProgressRootConcurrentErrors;
const exitStatus = renderRootSync(root, errorRetryLanes);
const recoverableErrors = workInProgressRootConcurrentErrors;
if (exitStatus !== RootErrored) {
// Successfully finished rendering
if (recoverableErrors !== null) {
// Although we recovered the UI without surfacing an error, we should
// still log the errors so they can be fixed.
for (let j = 0; j < recoverableErrors.length; j++) {
const recoverableError = recoverableErrors[j];
logRecoverableError(root.errorLoggingConfig, recoverableError);
// Successfully finished rendering on retry
if (errorsFromFirstAttempt !== null) {
// The errors from the failed first attempt have been recovered. Add
// them to the collection of recoverable errors. We'll log them in the
// commit phase.
if (workInProgressRootConcurrentErrors === null) {
workInProgressRootRecoverableErrors = errorsFromFirstAttempt;
} else {
workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply(
null,
errorsFromFirstAttempt,
);
}
}
} else {
Expand All @@ -929,7 +939,7 @@ function finishConcurrentRender(root, exitStatus, lanes) {
case RootErrored: {
// We should have already attempted to retry this tree. If we reached
// this point, it errored again. Commit it.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootSuspended: {
Expand Down Expand Up @@ -969,14 +979,14 @@ function finishConcurrentRender(root, exitStatus, lanes) {
// lower priority work to do. Instead of committing the fallback
// immediately, wait for more data to arrive.
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
msUntilTimeout,
);
break;
}
}
// The work expired. Commit immediately.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootSuspendedWithDelay: {
Expand Down Expand Up @@ -1007,20 +1017,20 @@ function finishConcurrentRender(root, exitStatus, lanes) {
// Instead of committing the fallback immediately, wait for more data
// to arrive.
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
msUntilTimeout,
);
break;
}
}

// Commit the placeholder.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
case RootCompleted: {
// The work completed. Ready to commit.
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);
break;
}
default: {
Expand Down Expand Up @@ -1140,7 +1150,7 @@ function performSyncWorkOnRoot(root) {
const finishedWork: Fiber = (root.current.alternate: any);
root.finishedWork = finishedWork;
root.finishedLanes = lanes;
commitRoot(root);
commitRoot(root, workInProgressRootRecoverableErrors);

// Before exiting, make sure there's a callback scheduled for the next
// pending level.
Expand Down Expand Up @@ -1337,6 +1347,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
workInProgressRootPingedLanes = NoLanes;
workInProgressRootConcurrentErrors = null;
workInProgressRootRecoverableErrors = null;

enqueueInterleavedUpdates();

Expand Down Expand Up @@ -1803,15 +1814,15 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
}
}

function commitRoot(root) {
function commitRoot(root: FiberRoot, recoverableErrors: null | Array<mixed>) {
// TODO: This no longer makes any sense. We already wrap the mutation and
// layout phases. Should be able to remove.
const previousUpdateLanePriority = getCurrentUpdatePriority();
const prevTransition = ReactCurrentBatchConfig.transition;
try {
ReactCurrentBatchConfig.transition = 0;
setCurrentUpdatePriority(DiscreteEventPriority);
commitRootImpl(root, previousUpdateLanePriority);
commitRootImpl(root, recoverableErrors, previousUpdateLanePriority);
} finally {
ReactCurrentBatchConfig.transition = prevTransition;
setCurrentUpdatePriority(previousUpdateLanePriority);
Expand All @@ -1820,7 +1831,11 @@ function commitRoot(root) {
return null;
}

function commitRootImpl(root, renderPriorityLevel) {
function commitRootImpl(
root: FiberRoot,
recoverableErrors: null | Array<mixed>,
renderPriorityLevel: EventPriority,
) {
do {
// `flushPassiveEffects` will call `flushSyncUpdateQueue` at the end, which
// means `flushPassiveEffects` will sometimes result in additional
Expand Down Expand Up @@ -2091,6 +2106,15 @@ function commitRootImpl(root, renderPriorityLevel) {
// additional work on this root is scheduled.
ensureRootIsScheduled(root, now());

if (recoverableErrors !== null) {
// There were errors during this render, but recovered from them without
// needing to surface it to the UI. We log them here.
for (let i = 0; i < recoverableErrors.length; i++) {
const recoverableError = recoverableErrors[i];
logRecoverableError(root.errorLoggingConfig, recoverableError);
}
}

if (hasUncaughtError) {
hasUncaughtError = false;
const error = firstUncaughtError;
Expand Down

0 comments on commit 5f37862

Please sign in to comment.