Fix: Infinite act loop caused by wrong shouldYield (#26317)
Based on a bug report from @bvaughn. `act` should not consult `shouldYield` when it's performing work, because in a unit testing environment, I/O (such as `setTimeout`) is likely mocked. So the result of `shouldYield` can't be trusted. In the regression test, I simulate the bug by mocking `shouldYield` to always return `true`. This causes an infinite loop in `act`, because it will keep trying to render and React will keep yielding.
Andrew Clark committed
Mar 5, 2023 at 20:24 UTC
49f7410467950de57bd1d9509e67de91185f7bad
2 files changed
+79
-1
packages/react-reconciler/src/ReactFiberWorkLoop.js
+11
-1
@@ -2268,7 +2268,17 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) {
2268
}
2269
}
2270
}
2271
- workLoopConcurrent();
2271
+
2272
+ if (__DEV__ && ReactCurrentActQueue.current !== null) {
2273
+ // `act` special case: If we're inside an `act` scope, don't consult
2274
+ // `shouldYield`. Always keep working until the render is complete.
2275
+ // This is not just an optimization: in a unit test environment, we
2276
+ // can't trust the result of `shouldYield`, because the host I/O is
2277
+ // likely mocked.
2278
+ workLoopSync();
2279
+ } else {
2280
+ workLoopConcurrent();
2281
+ }
2282
break;
2283
} catch (thrownValue) {
2284
handleThrow(root, thrownValue);
packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js
+68
@@ -320,3 +320,71 @@ describe(
320
});
321
},
322
);
323
+
324
+describe('`act` bypasses Scheduler methods completely,', () => {
325
+ let infiniteLoopGuard;
326
+
327
+ beforeEach(() => {
328
+ jest.resetModules();
329
+
330
+ infiniteLoopGuard = 0;
331
+
332
+ jest.mock('scheduler', () => {
333
+ const actual = jest.requireActual('scheduler/unstable_mock');
334
+ return {
335
+ ...actual,
336
+ unstable_shouldYield() {
337
+ // This simulates a bug report where `shouldYield` returns true in a
338
+ // unit testing environment. Because `act` will keep working until
339
+ // there's no more work left, it would fall into an infinite loop.
340
+ // The fix is that when performing work inside `act`, we should bypass
341
+ // `shouldYield` completely, because we can't trust it to be correct.
342
+ if (infiniteLoopGuard++ > 100) {
343
+ throw new Error('Detected an infinite loop');
344
+ }
345
+ return true;
346
+ },
347
+ };
348
+ });
349
+
350
+ React = require('react');
351
+ ReactNoop = require('react-noop-renderer');
352
+ startTransition = React.startTransition;
353
+ });
354
+
355
+ afterEach(() => {
356
+ jest.mock('scheduler', () => jest.requireActual('scheduler/unstable_mock'));
357
+ });
358
+
359
+ // @gate __DEV__
360
+ it('inside `act`, does not call `shouldYield`, even during a concurrent render', async () => {
361
+ function App() {
362
+ return (
363
+ <>
364
+ <div>A</div>
365
+ <div>B</div>
366
+ <div>C</div>
367
+ </>
368
+ );
369
+ }
370
+
371
+ const root = ReactNoop.createRoot();
372
+ const publicAct = React.unstable_act;
373
+ const prevIsReactActEnvironment = global.IS_REACT_ACT_ENVIRONMENT;
374
+ try {
375
+ global.IS_REACT_ACT_ENVIRONMENT = true;
376
+ await publicAct(async () => {
377
+ startTransition(() => root.render(<App />));
378
+ });
379
+ } finally {
380
+ global.IS_REACT_ACT_ENVIRONMENT = prevIsReactActEnvironment;
381
+ }
382
+ expect(root).toMatchRenderedOutput(
383
+ <>
384
+ <div>A</div>
385
+ <div>B</div>
386
+ <div>C</div>
387
+ </>,
388
+ );
389
+ });
390
+});