Skip to content

Commit 676e3ca

Browse files
author
Brian Vaughn
committed
Errors thrown by interaction tracking hooks use unhandledError to rethrow more safely.
Reverted try/finally change to ReactTestRendererScheduling
1 parent aaa8451 commit 676e3ca

File tree

2 files changed

+52
-57
lines changed

2 files changed

+52
-57
lines changed

packages/react-reconciler/src/ReactFiberScheduler.js

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -753,7 +753,6 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
753753
if (enableInteractionTracking) {
754754
__interactionsRef.current = prevInteractions;
755755

756-
let caughtError = null;
757756
let subscriber;
758757

759758
try {
@@ -766,7 +765,12 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
766765
subscriber.onWorkStopped(root.memoizedInteractions, threadID);
767766
}
768767
} catch (error) {
769-
caughtError = error;
768+
// It's not safe for commitRoot() to throw.
769+
// Store the error for now and we'll re-throw in finishRendering().
770+
if (!hasUnhandledError) {
771+
hasUnhandledError = true;
772+
unhandledError = error;
773+
}
770774
} finally {
771775
// Now that we're done, check the completed batch of interactions.
772776
// If no more work is outstanding for a given interaction,
@@ -777,14 +781,15 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
777781
try {
778782
subscriber.onInteractionScheduledWorkCompleted(interaction);
779783
} catch (error) {
780-
caughtError = caughtError || error;
784+
// It's not safe for commitRoot() to throw.
785+
// Store the error for now and we'll re-throw in finishRendering().
786+
if (!hasUnhandledError) {
787+
hasUnhandledError = true;
788+
unhandledError = error;
789+
}
781790
}
782791
}
783792
});
784-
785-
if (caughtError) {
786-
throw caughtError;
787-
}
788793
}
789794
}
790795
}
@@ -2161,6 +2166,8 @@ function performWorkOnRoot(
21612166
'by a bug in React. Please file an issue.',
21622167
);
21632168

2169+
isRendering = true;
2170+
21642171
if (enableInteractionTracking) {
21652172
// Determine which interactions this batch of work currently includes,
21662173
// So that we can accurately attribute time spent working on it,
@@ -2189,13 +2196,21 @@ function performWorkOnRoot(
21892196
expirationTime,
21902197
root.interactionThreadID,
21912198
);
2192-
subscriber.onWorkStarted(interactions, threadID);
2199+
try {
2200+
subscriber.onWorkStarted(interactions, threadID);
2201+
} catch (error) {
2202+
// Work thrown by a interaction-tracking subscriber should be rethrown,
2203+
// But only once it's safe (to avoid leaveing the scheduler in an invalid state).
2204+
// Store the error for now and we'll re-throw in finishRendering().
2205+
if (!hasUnhandledError) {
2206+
hasUnhandledError = true;
2207+
unhandledError = error;
2208+
}
2209+
}
21932210
}
21942211
}
21952212
}
21962213

2197-
isRendering = true;
2198-
21992214
// Check if this is async work or sync/expired work.
22002215
if (deadline === null || isExpired) {
22012216
// Flush work without yielding.

packages/react-test-renderer/src/ReactTestRendererScheduling.js

Lines changed: 27 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -35,27 +35,17 @@ export function flushAll(): Array<mixed> {
3535
yieldedValues = [];
3636
while (scheduledCallback !== null) {
3737
const cb = scheduledCallback;
38-
try {
39-
scheduledCallback = null;
40-
cb({
41-
timeRemaining() {
42-
// Keep rendering until there's no more work
43-
return 999;
44-
},
45-
// React's scheduler has its own way of keeping track of expired
46-
// work and doesn't read this, so don't bother setting it to the
47-
// correct value.
48-
didTimeout: false,
49-
});
50-
} catch (error) {
51-
// If an error occurred before we finished work,
52-
// Reset the callback so it can be flushed again.
53-
// Otherwise we leave the test renderer in a funky state,
54-
// Where nothing appears to be scheuduled unless new work is added,
55-
// And then the old failed work will also re-appear.
56-
scheduledCallback = cb;
57-
throw error;
58-
}
38+
scheduledCallback = null;
39+
cb({
40+
timeRemaining() {
41+
// Keep rendering until there's no more work
42+
return 999;
43+
},
44+
// React's scheduler has its own way of keeping track of expired
45+
// work and doesn't read this, so don't bother setting it to the
46+
// correct value.
47+
didTimeout: false,
48+
});
5949
}
6050
return yieldedValues;
6151
}
@@ -65,32 +55,22 @@ export function flushNumberOfYields(count: number): Array<mixed> {
6555
yieldedValues = [];
6656
while (scheduledCallback !== null && !didStop) {
6757
const cb = scheduledCallback;
68-
try {
69-
scheduledCallback = null;
70-
cb({
71-
timeRemaining() {
72-
if (yieldedValues.length >= count) {
73-
// We at least as many values as expected. Stop rendering.
74-
didStop = true;
75-
return 0;
76-
}
77-
// Keep rendering.
78-
return 999;
79-
},
80-
// React's scheduler has its own way of keeping track of expired
81-
// work and doesn't read this, so don't bother setting it to the
82-
// correct value.
83-
didTimeout: false,
84-
});
85-
} catch (error) {
86-
// If an error occurred before we finished work,
87-
// Reset the callback so it can be flushed again.
88-
// Otherwise we leave the test renderer in a funky state,
89-
// Where nothing appears to be scheuduled unless new work is added,
90-
// And then the old failed work will also re-appear.
91-
scheduledCallback = cb;
92-
throw error;
93-
}
58+
scheduledCallback = null;
59+
cb({
60+
timeRemaining() {
61+
if (yieldedValues.length >= count) {
62+
// We at least as many values as expected. Stop rendering.
63+
didStop = true;
64+
return 0;
65+
}
66+
// Keep rendering.
67+
return 999;
68+
},
69+
// React's scheduler has its own way of keeping track of expired
70+
// work and doesn't read this, so don't bother setting it to the
71+
// correct value.
72+
didTimeout: false,
73+
});
9474
}
9575
return yieldedValues;
9676
}

0 commit comments

Comments
 (0)