@samitouri / QOS-React-2 / commits / 6eadbe0c4a

Fix: Resolve entangled actions independently (#26726)

When there are multiple async actions at the same time, we entangle them together because we can't be sure which action an update might be associated with. (For this, we'd need AsyncContext.) However, if one of the async actions fails with an error, it should only affect that action, not all the other actions it may be entangled with. Resolving each action independently also means they can have independent pending state types, rather than being limited to an `isPending` boolean. We'll use this to implement an upcoming form API.

Andrew Clark committed Apr 25, 2023 at 20:43 UTC 6eadbe0c4aebf68410bb48147054ee22eec4c20c
5 files changed +278 -108
packages/react-reconciler/src/ReactFiberAsyncAction.js
+136 -91
@@ -7,44 +7,36 @@
7 * @flow
8 */
9
10 -import type {Wakeable} from 'shared/ReactTypes';
10 +import type {
11 + Thenable,
12 + PendingThenable,
13 + FulfilledThenable,
14 + RejectedThenable,
15 +} from 'shared/ReactTypes';
16 import type {Lane} from './ReactFiberLane';
12 -import {requestTransitionLane} from './ReactFiberRootScheduler';
13 -
14 -interface AsyncActionImpl {
15 - lane: Lane;
16 - listeners: Array<(false) => mixed>;
17 - count: number;
18 - then(
19 - onFulfill: (value: boolean) => mixed,
20 - onReject: (error: mixed) => mixed,
21 - ): void;
22 -}
23 -
24 -interface PendingAsyncAction extends AsyncActionImpl {
25 - status: 'pending';
26 -}
27 -
28 -interface FulfilledAsyncAction extends AsyncActionImpl {
29 - status: 'fulfilled';
30 - value: boolean;
31 -}
17
33 -interface RejectedAsyncAction extends AsyncActionImpl {
34 - status: 'rejected';
35 - reason: mixed;
36 -}
18 +import {requestTransitionLane} from './ReactFiberRootScheduler';
19 +import {NoLane} from './ReactFiberLane';
20
38 -type AsyncAction =
39 - | PendingAsyncAction
40 - | FulfilledAsyncAction
41 - | RejectedAsyncAction;
21 +// If there are multiple, concurrent async actions, they are entangled. All
22 +// transition updates that occur while the async action is still in progress
23 +// are treated as part of the action.
24 +//
25 +// The ideal behavior would be to treat each async function as an independent
26 +// action. However, without a mechanism like AsyncContext, we can't tell which
27 +// action an update corresponds to. So instead, we entangle them all into one.
28
43 -let currentAsyncAction: AsyncAction | null = null;
29 +// The listeners to notify once the entangled scope completes.
30 +let currentEntangledListeners: Array<() => mixed> | null = null;
31 +// The number of pending async actions in the entangled scope.
32 +let currentEntangledPendingCount: number = 0;
33 +// The transition lane shared by all updates in the entangled scope.
34 +let currentEntangledLane: Lane = NoLane;
35
45 -export function requestAsyncActionContext(
36 +export function requestAsyncActionContext<S>(
37 actionReturnValue: mixed,
47 -): AsyncAction | false {
38 + finishedState: S,
39 +): Thenable<S> | S {
40 if (
41 actionReturnValue !== null &&
42 typeof actionReturnValue === 'object' &&
@@ -53,78 +45,131 @@ export function requestAsyncActionContext(
45 // This is an async action.
46 //
47 // Return a thenable that resolves once the action scope (i.e. the async
56 - // function passed to startTransition) has finished running. The fulfilled
57 - // value is `false` to represent that the action is not pending.
58 - const thenable: Wakeable = (actionReturnValue: any);
59 - if (currentAsyncAction === null) {
48 + // function passed to startTransition) has finished running.
49 +
50 + const thenable: Thenable<mixed> = (actionReturnValue: any);
51 + let entangledListeners;
52 + if (currentEntangledListeners === null) {
53 // There's no outer async action scope. Create a new one.
61 - const asyncAction: AsyncAction = {
62 - lane: requestTransitionLane(),
63 - listeners: [],
64 - count: 0,
65 - status: 'pending',
66 - value: false,
67 - reason: undefined,
68 - then(resolve: boolean => mixed) {
69 - asyncAction.listeners.push(resolve);
70 - },
71 - };
72 - attachPingListeners(thenable, asyncAction);
73 - currentAsyncAction = asyncAction;
74 - return asyncAction;
54 + entangledListeners = currentEntangledListeners = [];
55 + currentEntangledPendingCount = 0;
56 + currentEntangledLane = requestTransitionLane();
57 } else {
76 - // Inherit the outer scope.
77 - const asyncAction: AsyncAction = (currentAsyncAction: any);
78 - attachPingListeners(thenable, asyncAction);
79 - return asyncAction;
58 + entangledListeners = currentEntangledListeners;
59 }
60 +
61 + currentEntangledPendingCount++;
62 + let resultStatus = 'pending';
63 + let rejectedReason;
64 + thenable.then(
65 + () => {
66 + resultStatus = 'fulfilled';
67 + pingEngtangledActionScope();
68 + },
69 + error => {
70 + resultStatus = 'rejected';
71 + rejectedReason = error;
72 + pingEngtangledActionScope();
73 + },
74 + );
75 +
76 + // Create a thenable that represents the result of this action, but doesn't
77 + // resolve until the entire entangled scope has finished.
78 + //
79 + // Expressed using promises:
80 + // const [thisResult] = await Promise.all([thisAction, entangledAction]);
81 + // return thisResult;
82 + const resultThenable = createResultThenable<S>(entangledListeners);
83 +
84 + // Attach a listener to fill in the result.
85 + entangledListeners.push(() => {
86 + switch (resultStatus) {
87 + case 'fulfilled': {
88 + const fulfilledThenable: FulfilledThenable<S> = (resultThenable: any);
89 + fulfilledThenable.status = 'fulfilled';
90 + fulfilledThenable.value = finishedState;
91 + break;
92 + }
93 + case 'rejected': {
94 + const rejectedThenable: RejectedThenable<S> = (resultThenable: any);
95 + rejectedThenable.status = 'rejected';
96 + rejectedThenable.reason = rejectedReason;
97 + break;
98 + }
99 + case 'pending':
100 + default: {
101 + // The listener above should have been called first, so `resultStatus`
102 + // should already be set to the correct value.
103 + throw new Error(
104 + 'Thenable should have already resolved. This ' +
105 + 'is a bug in React.',
106 + );
107 + }
108 + }
109 + });
110 +
111 + return resultThenable;
112 } else {
113 // This is not an async action, but it may be part of an outer async action.
83 - if (currentAsyncAction === null) {
84 - // There's no outer async action scope.
85 - return false;
114 + if (currentEntangledListeners === null) {
115 + return finishedState;
116 } else {
87 - // Inherit the outer scope.
88 - return currentAsyncAction;
117 + // Return a thenable that does not resolve until the entangled actions
118 + // have finished.
119 + const entangledListeners = currentEntangledListeners;
120 + const resultThenable = createResultThenable<S>(entangledListeners);
121 + entangledListeners.push(() => {
122 + const fulfilledThenable: FulfilledThenable<S> = (resultThenable: any);
123 + fulfilledThenable.status = 'fulfilled';
124 + fulfilledThenable.value = finishedState;
125 + });
126 + return resultThenable;
127 }
128 }
129 }
130
93 -export function peekAsyncActionContext(): AsyncAction | null {
94 - return currentAsyncAction;
131 +function pingEngtangledActionScope() {
132 + if (
133 + currentEntangledListeners !== null &&
134 + --currentEntangledPendingCount === 0
135 + ) {
136 + // All the actions have finished. Close the entangled async action scope
137 + // and notify all the listeners.
138 + const listeners = currentEntangledListeners;
139 + currentEntangledListeners = null;
140 + currentEntangledLane = NoLane;
141 + for (let i = 0; i < listeners.length; i++) {
142 + const listener = listeners[i];
143 + listener();
144 + }
145 + }
146 }
147
97 -function attachPingListeners(thenable: Wakeable, asyncAction: AsyncAction) {
98 - asyncAction.count++;
99 - thenable.then(
100 - () => {
101 - if (--asyncAction.count === 0) {
102 - const fulfilledAsyncAction: FulfilledAsyncAction = (asyncAction: any);
103 - fulfilledAsyncAction.status = 'fulfilled';
104 - completeAsyncActionScope(asyncAction);
105 - }
106 - },
107 - (error: mixed) => {
108 - if (--asyncAction.count === 0) {
109 - const rejectedAsyncAction: RejectedAsyncAction = (asyncAction: any);
110 - rejectedAsyncAction.status = 'rejected';
111 - rejectedAsyncAction.reason = error;
112 - completeAsyncActionScope(asyncAction);
113 - }
148 +function createResultThenable<S>(
149 + entangledListeners: Array<() => mixed>,
150 +): Thenable<S> {
151 + // Waits for the entangled async action to complete, then resolves to the
152 + // result of an individual action.
153 + const resultThenable: PendingThenable<S> = {
154 + status: 'pending',
155 + value: null,
156 + reason: null,
157 + then(resolve: S => mixed) {
158 + // This is a bit of a cheat. `resolve` expects a value of type `S` to be
159 + // passed, but because we're instrumenting the `status` field ourselves,
160 + // and we know this thenable will only be used by React, we also know
161 + // the value isn't actually needed. So we add the resolve function
162 + // directly to the entangled listeners.
163 + //
164 + // This is also why we don't need to check if the thenable is still
165 + // pending; the Suspense implementation already performs that check.
166 + const ping: () => mixed = (resolve: any);
167 + entangledListeners.push(ping);
168 },
115 - );
116 - return asyncAction;
169 + };
170 + return resultThenable;
171 }
172
119 -function completeAsyncActionScope(action: AsyncAction) {
120 - if (currentAsyncAction === action) {
121 - currentAsyncAction = null;
122 - }
123 -
124 - const listeners = action.listeners;
125 - action.listeners = [];
126 - for (let i = 0; i < listeners.length; i++) {
127 - const listener = listeners[i];
128 - listener(false);
129 - }
173 +export function peekEntangledActionLane(): Lane {
174 + return currentEntangledLane;
175 }
packages/react-reconciler/src/ReactFiberHooks.js
+19 -12
@@ -2431,8 +2431,10 @@ function updateDeferredValueImpl<T>(hook: Hook, prevValue: T, value: T): T {
2431 }
2432 }
2433
2434 -function startTransition(
2435 - setPending: (Thenable<boolean> | boolean) => void,
2434 +function startTransition<S>(
2435 + pendingState: S,
2436 + finishedState: S,
2437 + setPending: (Thenable<S> | S) => void,
2438 callback: () => mixed,
2439 options?: StartTransitionOptions,
2440 ): void {
@@ -2443,7 +2445,7 @@ function startTransition(
2445
2446 const prevTransition = ReactCurrentBatchConfig.transition;
2447 ReactCurrentBatchConfig.transition = null;
2446 - setPending(true);
2448 + setPending(pendingState);
2449 const currentTransition = (ReactCurrentBatchConfig.transition =
2450 ({}: BatchConfigTransition));
2451
@@ -2462,15 +2464,18 @@ function startTransition(
2464 if (enableAsyncActions) {
2465 const returnValue = callback();
2466
2465 - // `isPending` is either `false` or a thenable that resolves to `false`,
2466 - // depending on whether the action scope is an async function. In the
2467 - // async case, the resulting render will suspend until the async action
2468 - // scope has finished.
2469 - const isPending = requestAsyncActionContext(returnValue);
2470 - setPending(isPending);
2467 + // This is either `finishedState` or a thenable that resolves to
2468 + // `finishedState`, depending on whether the action scope is an async
2469 + // function. In the async case, the resulting render will suspend until
2470 + // the async action scope has finished.
2471 + const maybeThenable = requestAsyncActionContext(
2472 + returnValue,
2473 + finishedState,
2474 + );
2475 + setPending(maybeThenable);
2476 } else {
2477 // Async actions are not enabled.
2473 - setPending(false);
2478 + setPending(finishedState);
2479 callback();
2480 }
2481 } catch (error) {
@@ -2478,7 +2483,7 @@ function startTransition(
2483 // This is a trick to get the `useTransition` hook to rethrow the error.
2484 // When it unwraps the thenable with the `use` algorithm, the error
2485 // will be thrown.
2481 - const rejectedThenable: RejectedThenable<boolean> = {
2486 + const rejectedThenable: RejectedThenable<S> = {
2487 then() {},
2488 status: 'rejected',
2489 reason: error,
@@ -2594,6 +2599,8 @@ export function startHostTransition<F>(
2599 }
2600
2601 startTransition(
2602 + true,
2603 + false,
2604 setPending,
2605 // TODO: We can avoid this extra wrapper, somehow. Figure out layering
2606 // once more of this function is implemented.
@@ -2607,7 +2614,7 @@ function mountTransition(): [
2614 ] {
2615 const [, setPending] = mountState((false: Thenable<boolean> | boolean));
2616 // The `start` method never changes.
2610 - const start = startTransition.bind(null, setPending);
2617 + const start = startTransition.bind(null, true, false, setPending);
2618 const hook = mountWorkInProgressHook();
2619 hook.memoizedState = start;
2620 return [false, start];
packages/react-reconciler/src/ReactFiberWorkLoop.js
+4 -4
@@ -279,7 +279,7 @@ import {
279 requestTransitionLane,
280 } from './ReactFiberRootScheduler';
281 import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext';
282 -import {peekAsyncActionContext} from './ReactFiberAsyncAction';
282 +import {peekEntangledActionLane} from './ReactFiberAsyncAction';
283
284 const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map;
285
@@ -632,10 +632,10 @@ export function requestUpdateLane(fiber: Fiber): Lane {
632 transition._updatedFibers.add(fiber);
633 }
634
635 - const asyncAction = peekAsyncActionContext();
636 - return asyncAction !== null
635 + const actionScopeLane = peekEntangledActionLane();
636 + return actionScopeLane !== NoLane
637 ? // We're inside an async action scope. Reuse the same lane.
638 - asyncAction.lane
638 + actionScopeLane
639 : // We may or may not be inside an async action scope. If we are, this
640 // is the first update in that scope. Either way, we need to get a
641 // fresh transition lane.
packages/react-reconciler/src/__tests__/ReactAsyncActions-test.js
+117
@@ -527,4 +527,121 @@ describe('ReactAsyncActions', () => {
527 assertLog(['Pending: true', 'Pending: false']);
528 expect(root).toMatchRenderedOutput('Pending: false');
529 });
530 +
531 + // @gate enableAsyncActions
532 + test('if there are multiple entangled actions, and one of them errors, it only affects that action', async () => {
533 + class ErrorBoundary extends React.Component {
534 + state = {error: null};
535 + static getDerivedStateFromError(error) {
536 + return {error};
537 + }
538 + render() {
539 + if (this.state.error) {
540 + return <Text text={this.state.error.message} />;
541 + }
542 + return this.props.children;
543 + }
544 + }
545 +
546 + let startTransitionA;
547 + function ActionA() {
548 + const [isPendingA, start] = useTransition();
549 + startTransitionA = start;
550 + return <Text text={'Pending A: ' + isPendingA} />;
551 + }
552 +
553 + let startTransitionB;
554 + function ActionB() {
555 + const [isPending, start] = useTransition();
556 + startTransitionB = start;
557 + return <Text text={'Pending B: ' + isPending} />;
558 + }
559 +
560 + let startTransitionC;
561 + function ActionC() {
562 + const [isPending, start] = useTransition();
563 + startTransitionC = start;
564 + return <Text text={'Pending C: ' + isPending} />;
565 + }
566 +
567 + const root = ReactNoop.createRoot();
568 + await act(() => {
569 + root.render(
570 + <>
571 + <div>
572 + <ErrorBoundary>
573 + <ActionA />
574 + </ErrorBoundary>
575 + </div>
576 + <div>
577 + <ErrorBoundary>
578 + <ActionB />
579 + </ErrorBoundary>
580 + </div>
581 + <div>
582 + <ErrorBoundary>
583 + <ActionC />
584 + </ErrorBoundary>
585 + </div>
586 + </>,
587 + );
588 + });
589 + assertLog(['Pending A: false', 'Pending B: false', 'Pending C: false']);
590 + expect(root).toMatchRenderedOutput(
591 + <>
592 + <div>Pending A: false</div>
593 + <div>Pending B: false</div>
594 + <div>Pending C: false</div>
595 + </>,
596 + );
597 +
598 + // Start a bunch of entangled transitions. A and C throw errors, but B
599 + // doesn't. A and should surface their respective errors, but B should
600 + // finish successfully.
601 + await act(() => {
602 + startTransitionC(async () => {
603 + startTransitionB(async () => {
604 + startTransitionA(async () => {
605 + await getText('Wait for A');
606 + throw new Error('Oops A!');
607 + });
608 + await getText('Wait for B');
609 + });
610 + await getText('Wait for C');
611 + throw new Error('Oops C!');
612 + });
613 + });
614 + assertLog(['Pending A: true', 'Pending B: true', 'Pending C: true']);
615 +
616 + // Finish action A. We can't commit the result yet because it's entangled
617 + // with B and C.
618 + await act(() => resolveText('Wait for A'));
619 + assertLog([]);
620 +
621 + // Finish action B. Same as above.
622 + await act(() => resolveText('Wait for B'));
623 + assertLog([]);
624 +
625 + // Now finish action C. This is the last action in the entangled set, so
626 + // rendering can proceed.
627 + await act(() => resolveText('Wait for C'));
628 + assertLog([
629 + // A and C result in (separate) errors, but B does not.
630 + 'Oops A!',
631 + 'Pending B: false',
632 + 'Oops C!',
633 +
634 + // Because there was an error, React will try rendering one more time.
635 + 'Oops A!',
636 + 'Pending B: false',
637 + 'Oops C!',
638 + ]);
639 + expect(root).toMatchRenderedOutput(
640 + <>
641 + <div>Oops A!</div>
642 + <div>Pending B: false</div>
643 + <div>Oops C!</div>
644 + </>,
645 + );
646 + });
647 });
scripts/error-codes/codes.json
+2 -1
@@ -462,5 +462,6 @@
462 "474": "Suspense Exception: This is not a real error, and should not leak into userspace. If you're seeing this, it's likely a bug in React.",
463 "475": "Internal React Error: suspendedState null when it was expected to exists. Please report this as a React bug.",
464 "476": "Expected the form instance to be a HostComponent. This is a bug in React.",
465 - "477": "React Internal Error: processHintChunk is not implemented for Native-Relay. The fact that this method was called means there is a bug in React."
465 + "477": "React Internal Error: processHintChunk is not implemented for Native-Relay. The fact that this method was called means there is a bug in React.",
466 + "478": "Thenable should have already resolved. This is a bug in React."
467 }