Bugfix: Fix crash when suspending in shell during useSES re-render (#27199)
This adds a regression test for a bug where, after a store mutation, the updated data causes the shell of the app to suspend. When re-rendering due to a concurrent store mutation, we must check for the RootDidNotComplete exit status again. As a follow-up, I'm going to look into to cleaning up the places where we check the exit status, so bugs like these are less likely. I think we might be able to combine most of it into a single switch statement.
Andrew Clark committed
Aug 8, 2023 at 14:11 UTC
201becd3d294b38824930a818e3412c6e04ba2eb
2 files changed
+121
-61
packages/react-reconciler/src/ReactFiberWorkLoop.js
+39
-61
@@ -892,58 +892,39 @@ export function performConcurrentWorkOnRoot(
892
let exitStatus = shouldTimeSlice
893
? renderRootConcurrent(root, lanes)
894
: renderRootSync(root, lanes);
895
- if (exitStatus !== RootInProgress) {
896
- if (exitStatus === RootErrored) {
897
- // If something threw an error, try rendering one more time. We'll
898
- // render synchronously to block concurrent data mutations, and we'll
899
- // includes all pending updates are included. If it still fails after
900
- // the second attempt, we'll give up and commit the resulting tree.
901
- const originallyAttemptedLanes = lanes;
902
- const errorRetryLanes = getLanesToRetrySynchronouslyOnError(
903
- root,
904
- originallyAttemptedLanes,
905
- );
906
- if (errorRetryLanes !== NoLanes) {
907
- lanes = errorRetryLanes;
908
- exitStatus = recoverFromConcurrentError(
909
- root,
910
- originallyAttemptedLanes,
911
- errorRetryLanes,
912
- );
913
- }
914
- }
915
- if (exitStatus === RootFatalErrored) {
916
- const fatalError = workInProgressRootFatalError;
917
- prepareFreshStack(root, NoLanes);
918
- markRootSuspended(root, lanes);
919
- ensureRootIsScheduled(root);
920
- throw fatalError;
921
- }
895
923
- if (exitStatus === RootDidNotComplete) {
924
- // The render unwound without completing the tree. This happens in special
925
- // cases where need to exit the current render without producing a
926
- // consistent tree or committing.
927
- markRootSuspended(root, lanes);
928
- } else {
929
- // The render completed.
930
-
931
- // Check if this render may have yielded to a concurrent event, and if so,
932
- // confirm that any newly rendered stores are consistent.
933
- // TODO: It's possible that even a concurrent render may never have yielded
934
- // to the main thread, if it was fast enough, or if it expired. We could
935
- // skip the consistency check in that case, too.
936
- const renderWasConcurrent = !includesBlockingLane(root, lanes);
937
- const finishedWork: Fiber = (root.current.alternate: any);
938
- if (
939
- renderWasConcurrent &&
940
- !isRenderConsistentWithExternalStores(finishedWork)
941
- ) {
942
- // A store was mutated in an interleaved event. Render again,
943
- // synchronously, to block further mutations.
944
- exitStatus = renderRootSync(root, lanes);
896
+ if (exitStatus !== RootInProgress) {
897
+ let renderWasConcurrent = shouldTimeSlice;
898
+ do {
899
+ if (exitStatus === RootDidNotComplete) {
900
+ // The render unwound without completing the tree. This happens in special
901
+ // cases where need to exit the current render without producing a
902
+ // consistent tree or committing.
903
+ markRootSuspended(root, lanes);
904
+ } else {
905
+ // The render completed.
906
+
907
+ // Check if this render may have yielded to a concurrent event, and if so,
908
+ // confirm that any newly rendered stores are consistent.
909
+ // TODO: It's possible that even a concurrent render may never have yielded
910
+ // to the main thread, if it was fast enough, or if it expired. We could
911
+ // skip the consistency check in that case, too.
912
+ const finishedWork: Fiber = (root.current.alternate: any);
913
+ if (
914
+ renderWasConcurrent &&
915
+ !isRenderConsistentWithExternalStores(finishedWork)
916
+ ) {
917
+ // A store was mutated in an interleaved event. Render again,
918
+ // synchronously, to block further mutations.
919
+ exitStatus = renderRootSync(root, lanes);
920
+ // We assume the tree is now consistent because we didn't yield to any
921
+ // concurrent events.
922
+ renderWasConcurrent = false;
923
+ // Need to check the exit status again.
924
+ continue;
925
+ }
926
946
- // We need to check again if something threw
927
+ // Check if something threw
928
if (exitStatus === RootErrored) {
929
const originallyAttemptedLanes = lanes;
930
const errorRetryLanes = getLanesToRetrySynchronouslyOnError(
@@ -957,8 +938,7 @@ export function performConcurrentWorkOnRoot(
938
originallyAttemptedLanes,
939
errorRetryLanes,
940
);
960
- // We assume the tree is now consistent because we didn't yield to any
961
- // concurrent events.
941
+ renderWasConcurrent = false;
942
}
943
}
944
if (exitStatus === RootFatalErrored) {
@@ -969,16 +949,14 @@ export function performConcurrentWorkOnRoot(
949
throw fatalError;
950
}
951
972
- // FIXME: Need to check for RootDidNotComplete again. The factoring here
973
- // isn't ideal.
952
+ // We now have a consistent tree. The next step is either to commit it,
953
+ // or, if something suspended, wait to commit it after a timeout.
954
+ root.finishedWork = finishedWork;
955
+ root.finishedLanes = lanes;
956
+ finishConcurrentRender(root, exitStatus, finishedWork, lanes);
957
}
975
-
976
- // We now have a consistent tree. The next step is either to commit it,
977
- // or, if something suspended, wait to commit it after a timeout.
978
- root.finishedWork = finishedWork;
979
- root.finishedLanes = lanes;
980
- finishConcurrentRender(root, exitStatus, finishedWork, lanes);
981
- }
958
+ break;
959
+ } while (true);
960
}
961
962
ensureRootIsScheduled(root);
packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js
+82
@@ -19,6 +19,7 @@ let forwardRef;
19
let useImperativeHandle;
20
let useRef;
21
let useState;
22
+let use;
23
let startTransition;
24
let waitFor;
25
let waitForAll;
@@ -41,6 +42,7 @@ describe('useSyncExternalStore', () => {
42
forwardRef = React.forwardRef;
43
useRef = React.useRef;
44
useState = React.useState;
45
+ use = React.use;
46
useSyncExternalStore = React.useSyncExternalStore;
47
startTransition = React.startTransition;
48
@@ -212,4 +214,84 @@ describe('useSyncExternalStore', () => {
214
});
215
assertLog(['value:initial']);
216
});
217
+
218
+ test(
219
+ 'regression: suspending in shell after synchronously patching ' +
220
+ 'up store mutation',
221
+ async () => {
222
+ // Tests a case where a store is mutated during a concurrent event, then
223
+ // during the sync re-render, a synchronous render is triggered.
224
+
225
+ const store = createExternalStore('Initial');
226
+
227
+ let resolve;
228
+ const promise = new Promise(r => {
229
+ resolve = r;
230
+ });
231
+
232
+ function A() {
233
+ const value = useSyncExternalStore(store.subscribe, store.getState);
234
+
235
+ if (value === 'Updated') {
236
+ try {
237
+ use(promise);
238
+ } catch (x) {
239
+ Scheduler.log('Suspend A');
240
+ throw x;
241
+ }
242
+ }
243
+
244
+ return <Text text={'A: ' + value} />;
245
+ }
246
+
247
+ function B() {
248
+ const value = useSyncExternalStore(store.subscribe, store.getState);
249
+ return <Text text={'B: ' + value} />;
250
+ }
251
+
252
+ function App() {
253
+ return (
254
+ <>
255
+ <span>
256
+ <A />
257
+ </span>
258
+ <span>
259
+ <B />
260
+ </span>
261
+ </>
262
+ );
263
+ }
264
+
265
+ const root = ReactNoop.createRoot();
266
+ await act(async () => {
267
+ // A and B both read from the same store. Partially render A.
268
+ startTransition(() => root.render(<App />));
269
+ // A reads the initial value of the store.
270
+ await waitFor(['A: Initial']);
271
+
272
+ // Before B renders, mutate the store.
273
+ store.set('Updated');
274
+ });
275
+ assertLog([
276
+ // B reads the updated value of the store.
277
+ 'B: Updated',
278
+ // This should a synchronous re-render of A using the updated value. In
279
+ // this test, this causes A to suspend.
280
+ 'Suspend A',
281
+ ]);
282
+ // Nothing has committed, because A suspended and no fallback
283
+ // was provided.
284
+ expect(root).toMatchRenderedOutput(null);
285
+
286
+ // Resolve the data and finish rendering.
287
+ await act(() => resolve());
288
+ assertLog(['A: Updated', 'B: Updated']);
289
+ expect(root).toMatchRenderedOutput(
290
+ <>
291
+ <span>A: Updated</span>
292
+ <span>B: Updated</span>
293
+ </>,
294
+ );
295
+ },
296
+ );
297
});