Skip to content

Commit 073ecfd

Browse files
acdliteacdlite
committed
Don't log recoverable errors until commit phase
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.
1 parent 52281cc commit 073ecfd

File tree

2 files changed

+84
-36
lines changed

2 files changed

+84
-36
lines changed

‎packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
1414
import type {StackCursor} from './ReactFiberStack.new';
1515
import type {Flags} from './ReactFiberFlags';
1616
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new';
17+
import type {EventPriority} from './ReactEventPriorities.new';
1718

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

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

904+
const errorsFromFirstAttempt = workInProgressRootConcurrentErrors;
899905
const exitStatus = renderRootSync(root, errorRetryLanes);
900-
const recoverableErrors = workInProgressRootConcurrentErrors;
901906
if (exitStatus !== RootErrored) {
902-
// Successfully finished rendering
903-
if (recoverableErrors !== null) {
904-
// Although we recovered the UI without surfacing an error, we should
905-
// still log the errors so they can be fixed.
906-
for (let j = 0; j < recoverableErrors.length; j++) {
907-
const recoverableError = recoverableErrors[j];
908-
logRecoverableError(root.errorLoggingConfig, recoverableError);
907+
// Successfully finished rendering on retry
908+
if (errorsFromFirstAttempt !== null) {
909+
// The errors from the failed first attempt have been recovered. Add
910+
// them to the collection of recoverable errors. We'll log them in the
911+
// commit phase.
912+
if (workInProgressRootConcurrentErrors === null) {
913+
workInProgressRootRecoverableErrors = errorsFromFirstAttempt;
914+
} else {
915+
workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply(
916+
null,
917+
errorsFromFirstAttempt,
918+
);
909919
}
910920
}
911921
} else {
@@ -929,7 +939,7 @@ function finishConcurrentRender(root, exitStatus, lanes) {
929939
case RootErrored: {
930940
// We should have already attempted to retry this tree. If we reached
931941
// this point, it errored again. Commit it.
932-
commitRoot(root);
942+
commitRoot(root, workInProgressRootRecoverableErrors);
933943
break;
934944
}
935945
case RootSuspended: {
@@ -969,14 +979,14 @@ function finishConcurrentRender(root, exitStatus, lanes) {
969979
// lower priority work to do. Instead of committing the fallback
970980
// immediately, wait for more data to arrive.
971981
root.timeoutHandle = scheduleTimeout(
972-
commitRoot.bind(null, root),
982+
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
973983
msUntilTimeout,
974984
);
975985
break;
976986
}
977987
}
978988
// The work expired. Commit immediately.
979-
commitRoot(root);
989+
commitRoot(root, workInProgressRootRecoverableErrors);
980990
break;
981991
}
982992
case RootSuspendedWithDelay: {
@@ -1007,20 +1017,20 @@ function finishConcurrentRender(root, exitStatus, lanes) {
10071017
// Instead of committing the fallback immediately, wait for more data
10081018
// to arrive.
10091019
root.timeoutHandle = scheduleTimeout(
1010-
commitRoot.bind(null, root),
1020+
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
10111021
msUntilTimeout,
10121022
);
10131023
break;
10141024
}
10151025
}
10161026

10171027
// Commit the placeholder.
1018-
commitRoot(root);
1028+
commitRoot(root, workInProgressRootRecoverableErrors);
10191029
break;
10201030
}
10211031
case RootCompleted: {
10221032
// The work completed. Ready to commit.
1023-
commitRoot(root);
1033+
commitRoot(root, workInProgressRootRecoverableErrors);
10241034
break;
10251035
}
10261036
default: {
@@ -1140,7 +1150,7 @@ function performSyncWorkOnRoot(root) {
11401150
const finishedWork: Fiber = (root.current.alternate: any);
11411151
root.finishedWork = finishedWork;
11421152
root.finishedLanes = lanes;
1143-
commitRoot(root);
1153+
commitRoot(root, workInProgressRootRecoverableErrors);
11441154

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

13411352
enqueueInterleavedUpdates();
13421353

@@ -1803,15 +1814,15 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
18031814
}
18041815
}
18051816

1806-
function commitRoot(root) {
1817+
function commitRoot(root: FiberRoot, recoverableErrors: null | Array<mixed>) {
18071818
// TODO: This no longer makes any sense. We already wrap the mutation and
18081819
// layout phases. Should be able to remove.
18091820
const previousUpdateLanePriority = getCurrentUpdatePriority();
18101821
const prevTransition = ReactCurrentBatchConfig.transition;
18111822
try {
18121823
ReactCurrentBatchConfig.transition = 0;
18131824
setCurrentUpdatePriority(DiscreteEventPriority);
1814-
commitRootImpl(root, previousUpdateLanePriority);
1825+
commitRootImpl(root, recoverableErrors, previousUpdateLanePriority);
18151826
} finally {
18161827
ReactCurrentBatchConfig.transition = prevTransition;
18171828
setCurrentUpdatePriority(previousUpdateLanePriority);
@@ -1820,7 +1831,11 @@ function commitRoot(root) {
18201831
return null;
18211832
}
18221833

1823-
function commitRootImpl(root, renderPriorityLevel) {
1834+
function commitRootImpl(
1835+
root: FiberRoot,
1836+
recoverableErrors: null | Array<mixed>,
1837+
renderPriorityLevel: EventPriority,
1838+
) {
18241839
do {
18251840
// `flushPassiveEffects` will call `flushSyncUpdateQueue` at the end, which
18261841
// means `flushPassiveEffects` will sometimes result in additional
@@ -2091,6 +2106,15 @@ function commitRootImpl(root, renderPriorityLevel) {
20912106
// additional work on this root is scheduled.
20922107
ensureRootIsScheduled(root, now());
20932108

2109+
if (recoverableErrors !== null) {
2110+
// There were errors during this render, but recovered from them without
2111+
// needing to surface it to the UI. We log them here.
2112+
for (let i = 0; i < recoverableErrors.length; i++) {
2113+
const recoverableError = recoverableErrors[i];
2114+
logRecoverableError(root.errorLoggingConfig, recoverableError);
2115+
}
2116+
}
2117+
20942118
if (hasUncaughtError) {
20952119
hasUncaughtError = false;
20962120
const error = firstUncaughtError;

‎packages/react-reconciler/src/ReactFiberWorkLoop.old.js

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type {SuspenseState} from './ReactFiberSuspenseComponent.old';
1414
import type {StackCursor} from './ReactFiberStack.old';
1515
import type {Flags} from './ReactFiberFlags';
1616
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.old';
17+
import type {EventPriority} from './ReactEventPriorities.old';
1718

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

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

904+
const errorsFromFirstAttempt = workInProgressRootConcurrentErrors;
899905
const exitStatus = renderRootSync(root, errorRetryLanes);
900-
const recoverableErrors = workInProgressRootConcurrentErrors;
901906
if (exitStatus !== RootErrored) {
902-
// Successfully finished rendering
903-
if (recoverableErrors !== null) {
904-
// Although we recovered the UI without surfacing an error, we should
905-
// still log the errors so they can be fixed.
906-
for (let j = 0; j < recoverableErrors.length; j++) {
907-
const recoverableError = recoverableErrors[j];
908-
logRecoverableError(root.errorLoggingConfig, recoverableError);
907+
// Successfully finished rendering on retry
908+
if (errorsFromFirstAttempt !== null) {
909+
// The errors from the failed first attempt have been recovered. Add
910+
// them to the collection of recoverable errors. We'll log them in the
911+
// commit phase.
912+
if (workInProgressRootConcurrentErrors === null) {
913+
workInProgressRootRecoverableErrors = errorsFromFirstAttempt;
914+
} else {
915+
workInProgressRootConcurrentErrors = workInProgressRootConcurrentErrors.push.apply(
916+
null,
917+
errorsFromFirstAttempt,
918+
);
909919
}
910920
}
911921
} else {
@@ -929,7 +939,7 @@ function finishConcurrentRender(root, exitStatus, lanes) {
929939
case RootErrored: {
930940
// We should have already attempted to retry this tree. If we reached
931941
// this point, it errored again. Commit it.
932-
commitRoot(root);
942+
commitRoot(root, workInProgressRootRecoverableErrors);
933943
break;
934944
}
935945
case RootSuspended: {
@@ -969,14 +979,14 @@ function finishConcurrentRender(root, exitStatus, lanes) {
969979
// lower priority work to do. Instead of committing the fallback
970980
// immediately, wait for more data to arrive.
971981
root.timeoutHandle = scheduleTimeout(
972-
commitRoot.bind(null, root),
982+
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
973983
msUntilTimeout,
974984
);
975985
break;
976986
}
977987
}
978988
// The work expired. Commit immediately.
979-
commitRoot(root);
989+
commitRoot(root, workInProgressRootRecoverableErrors);
980990
break;
981991
}
982992
case RootSuspendedWithDelay: {
@@ -1007,20 +1017,20 @@ function finishConcurrentRender(root, exitStatus, lanes) {
10071017
// Instead of committing the fallback immediately, wait for more data
10081018
// to arrive.
10091019
root.timeoutHandle = scheduleTimeout(
1010-
commitRoot.bind(null, root),
1020+
commitRoot.bind(null, root, workInProgressRootRecoverableErrors),
10111021
msUntilTimeout,
10121022
);
10131023
break;
10141024
}
10151025
}
10161026

10171027
// Commit the placeholder.
1018-
commitRoot(root);
1028+
commitRoot(root, workInProgressRootRecoverableErrors);
10191029
break;
10201030
}
10211031
case RootCompleted: {
10221032
// The work completed. Ready to commit.
1023-
commitRoot(root);
1033+
commitRoot(root, workInProgressRootRecoverableErrors);
10241034
break;
10251035
}
10261036
default: {
@@ -1140,7 +1150,7 @@ function performSyncWorkOnRoot(root) {
11401150
const finishedWork: Fiber = (root.current.alternate: any);
11411151
root.finishedWork = finishedWork;
11421152
root.finishedLanes = lanes;
1143-
commitRoot(root);
1153+
commitRoot(root, workInProgressRootRecoverableErrors);
11441154

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

13411352
enqueueInterleavedUpdates();
13421353

@@ -1803,15 +1814,15 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
18031814
}
18041815
}
18051816

1806-
function commitRoot(root) {
1817+
function commitRoot(root: FiberRoot, recoverableErrors: null | Array<mixed>) {
18071818
// TODO: This no longer makes any sense. We already wrap the mutation and
18081819
// layout phases. Should be able to remove.
18091820
const previousUpdateLanePriority = getCurrentUpdatePriority();
18101821
const prevTransition = ReactCurrentBatchConfig.transition;
18111822
try {
18121823
ReactCurrentBatchConfig.transition = 0;
18131824
setCurrentUpdatePriority(DiscreteEventPriority);
1814-
commitRootImpl(root, previousUpdateLanePriority);
1825+
commitRootImpl(root, recoverableErrors, previousUpdateLanePriority);
18151826
} finally {
18161827
ReactCurrentBatchConfig.transition = prevTransition;
18171828
setCurrentUpdatePriority(previousUpdateLanePriority);
@@ -1820,7 +1831,11 @@ function commitRoot(root) {
18201831
return null;
18211832
}
18221833

1823-
function commitRootImpl(root, renderPriorityLevel) {
1834+
function commitRootImpl(
1835+
root: FiberRoot,
1836+
recoverableErrors: null | Array<mixed>,
1837+
renderPriorityLevel: EventPriority,
1838+
) {
18241839
do {
18251840
// `flushPassiveEffects` will call `flushSyncUpdateQueue` at the end, which
18261841
// means `flushPassiveEffects` will sometimes result in additional
@@ -2091,6 +2106,15 @@ function commitRootImpl(root, renderPriorityLevel) {
20912106
// additional work on this root is scheduled.
20922107
ensureRootIsScheduled(root, now());
20932108

2109+
if (recoverableErrors !== null) {
2110+
// There were errors during this render, but recovered from them without
2111+
// needing to surface it to the UI. We log them here.
2112+
for (let i = 0; i < recoverableErrors.length; i++) {
2113+
const recoverableError = recoverableErrors[i];
2114+
logRecoverableError(root.errorLoggingConfig, recoverableError);
2115+
}
2116+
}
2117+
20942118
if (hasUncaughtError) {
20952119
hasUncaughtError = false;
20962120
const error = firstUncaughtError;

0 commit comments

Comments
 (0)