Skip to content

Commit a155860

Browse files
acdliteacdlite
authored
Fix: Don't flush discrete at end of batchedUpdates (#21229)
The outermost `batchedUpdates` call flushes pending sync updates at the end. This was intended for legacy sync mode, but it also happens to flush discrete updates in concurrent mode. Instead, we should only flush sync updates at the end of `batchedUpdates` for legacy roots. Discrete sync updates can wait to flush in the microtask. `discreteUpdates` has the same issue, which is how I originally noticed this, but I'll change that one in a separate commit since it requires updating a few (no longer relevant) internal tests.
1 parent 89847bf commit a155860

File tree

8 files changed

+198
-71
lines changed

8 files changed

+198
-71
lines changed

‎packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ describe('StoreStressConcurrent', () => {
1111
let React;
1212
let ReactDOM;
1313
let act;
14+
let actAsync;
1415
let bridge;
1516
let store;
1617
let print;
@@ -23,6 +24,9 @@ describe('StoreStressConcurrent', () => {
2324
React = require('react');
2425
ReactDOM = require('react-dom');
2526
act = require('./utils').act;
27+
// TODO: Figure out recommendation for concurrent mode tests, then replace
28+
// this helper with the real thing.
29+
actAsync = require('./utils').actAsync;
2630

2731
print = require('./storeSerializer').print;
2832
});
@@ -758,7 +762,7 @@ describe('StoreStressConcurrent', () => {
758762

759763
// Force fallback.
760764
expect(print(store)).toEqual(snapshots[i]);
761-
act(() => {
765+
await actAsync(async () => {
762766
bridge.send('overrideSuspense', {
763767
id: suspenseID,
764768
rendererID: store.getRendererIDForElement(suspenseID),
@@ -768,7 +772,7 @@ describe('StoreStressConcurrent', () => {
768772
expect(print(store)).toEqual(snapshots[j]);
769773

770774
// Stop forcing fallback.
771-
act(() => {
775+
await actAsync(async () => {
772776
bridge.send('overrideSuspense', {
773777
id: suspenseID,
774778
rendererID: store.getRendererIDForElement(suspenseID),
@@ -818,7 +822,7 @@ describe('StoreStressConcurrent', () => {
818822
expect(print(store)).toEqual(snapshots[j]);
819823

820824
// Stop forcing fallback. This reverts to primary content.
821-
act(() => {
825+
await actAsync(async () => {
822826
bridge.send('overrideSuspense', {
823827
id: suspenseID,
824828
rendererID: store.getRendererIDForElement(suspenseID),
@@ -829,13 +833,13 @@ describe('StoreStressConcurrent', () => {
829833
expect(print(store)).toEqual(snapshots[i]);
830834

831835
// Clean up after every iteration.
832-
act(() => root.unmount());
836+
await actAsync(async () => root.unmount());
833837
expect(print(store)).toBe('');
834838
}
835839
}
836840
});
837841

838-
it('should handle a stress test for Suspense without type change (Concurrent Mode)', () => {
842+
it('should handle a stress test for Suspense without type change (Concurrent Mode)', async () => {
839843
const A = () => 'a';
840844
const B = () => 'b';
841845
const C = () => 'c';
@@ -1294,7 +1298,7 @@ describe('StoreStressConcurrent', () => {
12941298

12951299
// Force fallback.
12961300
expect(print(store)).toEqual(snapshots[i]);
1297-
act(() => {
1301+
await actAsync(async () => {
12981302
bridge.send('overrideSuspense', {
12991303
id: suspenseID,
13001304
rendererID: store.getRendererIDForElement(suspenseID),
@@ -1304,7 +1308,7 @@ describe('StoreStressConcurrent', () => {
13041308
expect(print(store)).toEqual(fallbackSnapshots[j]);
13051309

13061310
// Stop forcing fallback.
1307-
act(() => {
1311+
await actAsync(async () => {
13081312
bridge.send('overrideSuspense', {
13091313
id: suspenseID,
13101314
rendererID: store.getRendererIDForElement(suspenseID),
@@ -1354,7 +1358,7 @@ describe('StoreStressConcurrent', () => {
13541358
expect(print(store)).toEqual(fallbackSnapshots[j]);
13551359

13561360
// Stop forcing fallback. This reverts to primary content.
1357-
act(() => {
1361+
await actAsync(async () => {
13581362
bridge.send('overrideSuspense', {
13591363
id: suspenseID,
13601364
rendererID: store.getRendererIDForElement(suspenseID),

‎packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,4 +371,54 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
371371
);
372372
}
373373
});
374+
375+
// @gate experimental
376+
it('should not flush discrete events at the end of outermost batchedUpdates', async () => {
377+
const root = ReactDOM.unstable_createRoot(container);
378+
379+
let target;
380+
function Foo() {
381+
const [count, setCount] = React.useState(0);
382+
return (
383+
<div
384+
ref={el => {
385+
target = el;
386+
if (target !== null) {
387+
el.onclick = () => {
388+
ReactDOM.unstable_batchedUpdates(() => {
389+
setCount(count + 1);
390+
});
391+
Scheduler.unstable_yieldValue(
392+
container.textContent + ' [after batchedUpdates]',
393+
);
394+
};
395+
}
396+
}}>
397+
Count: {count}
398+
</div>
399+
);
400+
}
401+
402+
await act(async () => {
403+
root.render(<Foo />);
404+
});
405+
expect(container.textContent).toEqual('Count: 0');
406+
407+
const pressEvent = document.createEvent('Event');
408+
pressEvent.initEvent('click', true, true);
409+
dispatchAndSetCurrentEvent(target, pressEvent);
410+
411+
expect(Scheduler).toHaveYielded(['Count: 0 [after batchedUpdates]']);
412+
// TODO: There's a `flushDiscreteUpdates` call at the end of the event
413+
// delegation listener that gets called even if no React event handlers are
414+
// fired. Once that is removed, this will be 0, not 1.
415+
// expect(container.textContent).toEqual('Count: 0');
416+
expect(container.textContent).toEqual('Count: 1');
417+
418+
// Intentionally not using `act` so we can observe in between the click
419+
// event and the microtask, without batching.
420+
await null;
421+
422+
expect(container.textContent).toEqual('Count: 1');
423+
});
374424
});

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
import {ImmediatePriority, scheduleCallback} from './Scheduler';
1818

1919
let syncQueue: Array<SchedulerCallback> | null = null;
20+
let includesLegacySyncCallbacks: boolean = false;
2021
let isFlushingSyncQueue: boolean = false;
2122

2223
export function scheduleSyncCallback(callback: SchedulerCallback) {
@@ -31,7 +32,23 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
3132
}
3233
}
3334

34-
export function flushSyncCallbackQueue() {
35+
export function scheduleLegacySyncCallback(callback: SchedulerCallback) {
36+
includesLegacySyncCallbacks = true;
37+
scheduleSyncCallback(callback);
38+
}
39+
40+
export function flushSyncCallbacksOnlyInLegacyMode() {
41+
// Only flushes the queue if there's a legacy sync callback scheduled.
42+
// TODO: There's only a single type of callback: performSyncOnWorkOnRoot. So
43+
// it might make more sense for the queue to be a list of roots instead of a
44+
// list of generic callbacks. Then we can have two: one for legacy roots, one
45+
// for concurrent roots. And this method would only flush the legacy ones.
46+
if (includesLegacySyncCallbacks) {
47+
flushSyncCallbacks();
48+
}
49+
}
50+
51+
export function flushSyncCallbacks() {
3552
if (!isFlushingSyncQueue && syncQueue !== null) {
3653
// Prevent re-entrancy.
3754
isFlushingSyncQueue = true;
@@ -50,13 +67,14 @@ export function flushSyncCallbackQueue() {
5067
} while (callback !== null);
5168
}
5269
syncQueue = null;
70+
includesLegacySyncCallbacks = false;
5371
} catch (error) {
5472
// If something throws, leave the remaining callbacks on the queue.
5573
if (syncQueue !== null) {
5674
syncQueue = syncQueue.slice(i + 1);
5775
}
5876
// Resume flushing in the next tick
59-
scheduleCallback(ImmediatePriority, flushSyncCallbackQueue);
77+
scheduleCallback(ImmediatePriority, flushSyncCallbacks);
6078
throw error;
6179
} finally {
6280
setCurrentUpdatePriority(previousUpdatePriority);

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
import {ImmediatePriority, scheduleCallback} from './Scheduler';
1818

1919
let syncQueue: Array<SchedulerCallback> | null = null;
20+
let includesLegacySyncCallbacks: boolean = false;
2021
let isFlushingSyncQueue: boolean = false;
2122

2223
export function scheduleSyncCallback(callback: SchedulerCallback) {
@@ -31,7 +32,23 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
3132
}
3233
}
3334

34-
export function flushSyncCallbackQueue() {
35+
export function scheduleLegacySyncCallback(callback: SchedulerCallback) {
36+
includesLegacySyncCallbacks = true;
37+
scheduleSyncCallback(callback);
38+
}
39+
40+
export function flushSyncCallbacksOnlyInLegacyMode() {
41+
// Only flushes the queue if there's a legacy sync callback scheduled.
42+
// TODO: There's only a single type of callback: performSyncOnWorkOnRoot. So
43+
// it might make more sense for the queue to be a list of roots instead of a
44+
// list of generic callbacks. Then we can have two: one for legacy roots, one
45+
// for concurrent roots. And this method would only flush the legacy ones.
46+
if (includesLegacySyncCallbacks) {
47+
flushSyncCallbacks();
48+
}
49+
}
50+
51+
export function flushSyncCallbacks() {
3552
if (!isFlushingSyncQueue && syncQueue !== null) {
3653
// Prevent re-entrancy.
3754
isFlushingSyncQueue = true;
@@ -50,13 +67,14 @@ export function flushSyncCallbackQueue() {
5067
} while (callback !== null);
5168
}
5269
syncQueue = null;
70+
includesLegacySyncCallbacks = false;
5371
} catch (error) {
5472
// If something throws, leave the remaining callbacks on the queue.
5573
if (syncQueue !== null) {
5674
syncQueue = syncQueue.slice(i + 1);
5775
}
5876
// Resume flushing in the next tick
59-
scheduleCallback(ImmediatePriority, flushSyncCallbackQueue);
77+
scheduleCallback(ImmediatePriority, flushSyncCallbacks);
6078
throw error;
6179
} finally {
6280
setCurrentUpdatePriority(previousUpdatePriority);

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

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,10 @@ import {
4949
IdlePriority as IdleSchedulerPriority,
5050
} from './Scheduler';
5151
import {
52-
flushSyncCallbackQueue,
52+
flushSyncCallbacks,
53+
flushSyncCallbacksOnlyInLegacyMode,
5354
scheduleSyncCallback,
55+
scheduleLegacySyncCallback,
5456
} from './ReactFiberSyncTaskQueue.new';
5557
import {
5658
NoFlags as NoHookEffect,
@@ -561,7 +563,7 @@ export function scheduleUpdateOnFiber(
561563
// without immediately flushing it. We only do this for user-initiated
562564
// updates, to preserve historical behavior of legacy mode.
563565
resetRenderTimer();
564-
flushSyncCallbackQueue();
566+
flushSyncCallbacksOnlyInLegacyMode();
565567
}
566568
}
567569
} else {
@@ -698,13 +700,17 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
698700
if (newCallbackPriority === SyncLane) {
699701
// Special case: Sync React callbacks are scheduled on a special
700702
// internal queue
701-
scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root));
703+
if (root.tag === LegacyRoot) {
704+
scheduleLegacySyncCallback(performSyncWorkOnRoot.bind(null, root));
705+
} else {
706+
scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root));
707+
}
702708
if (supportsMicrotasks) {
703709
// Flush the queue in a microtask.
704-
scheduleMicrotask(flushSyncCallbackQueue);
710+
scheduleMicrotask(flushSyncCallbacks);
705711
} else {
706712
// Flush the queue in an Immediate task.
707-
scheduleCallback(ImmediateSchedulerPriority, flushSyncCallbackQueue);
713+
scheduleCallback(ImmediateSchedulerPriority, flushSyncCallbacks);
708714
}
709715
newCallbackNode = null;
710716
} else {
@@ -1054,7 +1060,7 @@ export function flushRoot(root: FiberRoot, lanes: Lanes) {
10541060
ensureRootIsScheduled(root, now());
10551061
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
10561062
resetRenderTimer();
1057-
flushSyncCallbackQueue();
1063+
flushSyncCallbacks();
10581064
}
10591065
}
10601066
}
@@ -1085,7 +1091,7 @@ export function flushDiscreteUpdates() {
10851091
// like `el.focus()`. Exit.
10861092
return;
10871093
}
1088-
flushSyncCallbackQueue();
1094+
flushSyncCallbacks();
10891095
// If the discrete updates scheduled passive effects, flush them now so that
10901096
// they fire before the next serial event.
10911097
flushPassiveEffects();
@@ -1111,10 +1117,11 @@ export function batchedUpdates<A, R>(fn: A => R, a: A): R {
11111117
return fn(a);
11121118
} finally {
11131119
executionContext = prevExecutionContext;
1120+
// If there were legacy sync updates, flush them at the end of the outer
1121+
// most batchedUpdates-like method.
11141122
if (executionContext === NoContext) {
1115-
// Flush the immediate callbacks that were scheduled during this batch
11161123
resetRenderTimer();
1117-
flushSyncCallbackQueue();
1124+
flushSyncCallbacksOnlyInLegacyMode();
11181125
}
11191126
}
11201127
}
@@ -1126,10 +1133,11 @@ export function batchedEventUpdates<A, R>(fn: A => R, a: A): R {
11261133
return fn(a);
11271134
} finally {
11281135
executionContext = prevExecutionContext;
1136+
// If there were legacy sync updates, flush them at the end of the outer
1137+
// most batchedUpdates-like method.
11291138
if (executionContext === NoContext) {
1130-
// Flush the immediate callbacks that were scheduled during this batch
11311139
resetRenderTimer();
1132-
flushSyncCallbackQueue();
1140+
flushSyncCallbacksOnlyInLegacyMode();
11331141
}
11341142
}
11351143
}
@@ -1151,9 +1159,10 @@ export function discreteUpdates<A, B, C, D, R>(
11511159
setCurrentUpdatePriority(previousPriority);
11521160
ReactCurrentBatchConfig.transition = prevTransition;
11531161
if (executionContext === NoContext) {
1154-
// Flush the immediate callbacks that were scheduled during this batch
11551162
resetRenderTimer();
1156-
flushSyncCallbackQueue();
1163+
// TODO: This should only flush legacy sync updates. Not discrete updates
1164+
// in Concurrent Mode. Discrete updates will flush in a microtask.
1165+
flushSyncCallbacks();
11571166
}
11581167
}
11591168
}
@@ -1166,10 +1175,13 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {
11661175
return fn(a);
11671176
} finally {
11681177
executionContext = prevExecutionContext;
1178+
// If there were legacy sync updates, flush them at the end of the outer
1179+
// most batchedUpdates-like method.
11691180
if (executionContext === NoContext) {
1170-
// Flush the immediate callbacks that were scheduled during this batch
11711181
resetRenderTimer();
1172-
flushSyncCallbackQueue();
1182+
// TODO: I think this call is redundant, because we flush inside
1183+
// scheduleUpdateOnFiber when LegacyUnbatchedContext is set.
1184+
flushSyncCallbacksOnlyInLegacyMode();
11731185
}
11741186
}
11751187
}
@@ -1196,7 +1208,7 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
11961208
// Note that this will happen even if batchedUpdates is higher up
11971209
// the stack.
11981210
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
1199-
flushSyncCallbackQueue();
1211+
flushSyncCallbacks();
12001212
} else {
12011213
if (__DEV__) {
12021214
console.error(
@@ -1226,7 +1238,7 @@ export function flushControlled(fn: () => mixed): void {
12261238
if (executionContext === NoContext) {
12271239
// Flush the immediate callbacks that were scheduled during this batch
12281240
resetRenderTimer();
1229-
flushSyncCallbackQueue();
1241+
flushSyncCallbacks();
12301242
}
12311243
}
12321244
}
@@ -2098,7 +2110,7 @@ function commitRootImpl(root, renderPriorityLevel) {
20982110
}
20992111

21002112
// If layout work was scheduled, flush it now.
2101-
flushSyncCallbackQueue();
2113+
flushSyncCallbacks();
21022114

21032115
if (__DEV__) {
21042116
if (enableDebugTracing) {
@@ -2224,7 +2236,7 @@ function flushPassiveEffectsImpl() {
22242236

22252237
executionContext = prevExecutionContext;
22262238

2227-
flushSyncCallbackQueue();
2239+
flushSyncCallbacks();
22282240

22292241
// If additional passive effects were scheduled, increment a counter. If this
22302242
// exceeds the limit, we'll fire a warning.

0 commit comments

Comments
 (0)