Throttle retries even if everything has loaded (#26611)
If a Suspense fallback is shown, and the data finishes loading really quickly after that, we throttle the content from appearing for 500ms to reduce thrash. This already works for successive fallback states (like if one fallback is nested inside another) but it wasn't being applied to the final step in the sequence: if there were no more unresolved Suspense boundaries in the tree, the content would appear immediately. This fixes the throttling behavior so that it applies to all renders that are the result of suspended data being loaded. (Our internal jargon term for this is a "retry".)
Andrew Clark committed
Apr 12, 2023 at 20:25 UTC
8256781fdf3ae1947a7f27ddc78ae11b9989c2cd
2 files changed
+70
-152
packages/react-reconciler/src/ReactFiberWorkLoop.js
+66
-92
@@ -1085,108 +1085,87 @@ function finishConcurrentRender(
1085
finishedWork: Fiber,
1086
lanes: Lanes,
1087
) {
1088
+ // TODO: The fact that most of these branches are identical suggests that some
1089
+ // of the exit statuses are not best modeled as exit statuses and should be
1090
+ // tracked orthogonally.
1091
switch (exitStatus) {
1092
case RootInProgress:
1093
case RootFatalErrored: {
1094
throw new Error('Root did not complete. This is a bug in React.');
1095
}
1093
- case RootErrored: {
1094
- // We should have already attempted to retry this tree. If we reached
1095
- // this point, it errored again. Commit it.
1096
- commitRootWhenReady(
1097
- root,
1098
- finishedWork,
1099
- workInProgressRootRecoverableErrors,
1100
- workInProgressTransitions,
1101
- lanes,
1102
- );
1103
- break;
1104
- }
1105
- case RootSuspended: {
1106
- markRootSuspended(root, lanes);
1107
-
1108
- // We have an acceptable loading state. We need to figure out if we
1109
- // should immediately commit it or wait a bit.
1110
-
1111
- if (
1112
- includesOnlyRetries(lanes) &&
1113
- // do not delay if we're inside an act() scope
1114
- !shouldForceFlushFallbacksInDEV()
1115
- ) {
1116
- // This render only included retries, no updates. Throttle committing
1117
- // retries so that we don't show too many loading states too quickly.
1118
- const msUntilTimeout =
1119
- globalMostRecentFallbackTime + FALLBACK_THROTTLE_MS - now();
1120
- // Don't bother with a very short suspense time.
1121
- if (msUntilTimeout > 10) {
1122
- const nextLanes = getNextLanes(root, NoLanes);
1123
- if (nextLanes !== NoLanes) {
1124
- // There's additional work on this root.
1125
- break;
1126
- }
1127
-
1128
- // The render is suspended, it hasn't timed out, and there's no
1129
- // lower priority work to do. Instead of committing the fallback
1130
- // immediately, wait for more data to arrive.
1131
- root.timeoutHandle = scheduleTimeout(
1132
- commitRootWhenReady.bind(
1133
- null,
1134
- root,
1135
- finishedWork,
1136
- workInProgressRootRecoverableErrors,
1137
- workInProgressTransitions,
1138
- lanes,
1139
- ),
1140
- msUntilTimeout,
1141
- );
1142
- break;
1143
- }
1144
- }
1145
- // The work expired. Commit immediately.
1146
- commitRootWhenReady(
1147
- root,
1148
- finishedWork,
1149
- workInProgressRootRecoverableErrors,
1150
- workInProgressTransitions,
1151
- lanes,
1152
- );
1153
- break;
1154
- }
1096
case RootSuspendedWithDelay: {
1156
- markRootSuspended(root, lanes);
1157
-
1097
if (includesOnlyTransitions(lanes)) {
1098
// This is a transition, so we should exit without committing a
1099
// placeholder and without scheduling a timeout. Delay indefinitely
1100
// until we receive more data.
1162
- break;
1101
+ markRootSuspended(root, lanes);
1102
+ return;
1103
}
1164
-
1104
// Commit the placeholder.
1166
- commitRootWhenReady(
1167
- root,
1168
- finishedWork,
1169
- workInProgressRootRecoverableErrors,
1170
- workInProgressTransitions,
1171
- lanes,
1172
- );
1105
break;
1106
}
1107
+ case RootErrored:
1108
+ case RootSuspended:
1109
case RootCompleted: {
1176
- // The work completed.
1177
- commitRootWhenReady(
1178
- root,
1179
- finishedWork,
1180
- workInProgressRootRecoverableErrors,
1181
- workInProgressTransitions,
1182
- lanes,
1183
- );
1110
break;
1111
}
1112
default: {
1113
throw new Error('Unknown root exit status.');
1114
}
1115
}
1116
+
1117
+ if (shouldForceFlushFallbacksInDEV()) {
1118
+ // We're inside an `act` scope. Commit immediately.
1119
+ commitRoot(
1120
+ root,
1121
+ workInProgressRootRecoverableErrors,
1122
+ workInProgressTransitions,
1123
+ );
1124
+ } else {
1125
+ if (includesOnlyRetries(lanes)) {
1126
+ // This render only included retries, no updates. Throttle committing
1127
+ // retries so that we don't show too many loading states too quickly.
1128
+ const msUntilTimeout =
1129
+ globalMostRecentFallbackTime + FALLBACK_THROTTLE_MS - now();
1130
+
1131
+ // Don't bother with a very short suspense time.
1132
+ if (msUntilTimeout > 10) {
1133
+ markRootSuspended(root, lanes);
1134
+
1135
+ const nextLanes = getNextLanes(root, NoLanes);
1136
+ if (nextLanes !== NoLanes) {
1137
+ // There's additional work we can do on this root. We might as well
1138
+ // attempt to work on that while we're suspended.
1139
+ return;
1140
+ }
1141
+
1142
+ // The render is suspended, it hasn't timed out, and there's no
1143
+ // lower priority work to do. Instead of committing the fallback
1144
+ // immediately, wait for more data to arrive.
1145
+ // TODO: Combine retry throttling with Suspensey commits. Right now they
1146
+ // run one after the other.
1147
+ root.timeoutHandle = scheduleTimeout(
1148
+ commitRootWhenReady.bind(
1149
+ null,
1150
+ root,
1151
+ finishedWork,
1152
+ workInProgressRootRecoverableErrors,
1153
+ workInProgressTransitions,
1154
+ lanes,
1155
+ ),
1156
+ msUntilTimeout,
1157
+ );
1158
+ return;
1159
+ }
1160
+ }
1161
+ commitRootWhenReady(
1162
+ root,
1163
+ finishedWork,
1164
+ workInProgressRootRecoverableErrors,
1165
+ workInProgressTransitions,
1166
+ lanes,
1167
+ );
1168
+ }
1169
}
1170
1171
function commitRootWhenReady(
@@ -1196,6 +1175,8 @@ function commitRootWhenReady(
1175
transitions: Array<Transition> | null,
1176
lanes: Lanes,
1177
) {
1178
+ // TODO: Combine retry throttling with Suspensey commits. Right now they run
1179
+ // one after the other.
1180
if (includesOnlyNonUrgentLanes(lanes)) {
1181
// Before committing, ask the renderer whether the host tree is ready.
1182
// If it's not, we'll wait until it notifies us.
@@ -1218,22 +1199,15 @@ function commitRootWhenReady(
1199
// us that it's ready. This will be canceled if we start work on the
1200
// root again.
1201
root.cancelPendingCommit = schedulePendingCommit(
1221
- commitRoot.bind(
1222
- null,
1223
- root,
1224
- workInProgressRootRecoverableErrors,
1225
- workInProgressTransitions,
1226
- ),
1202
+ commitRoot.bind(null, root, recoverableErrors, transitions),
1203
);
1204
+ markRootSuspended(root, lanes);
1205
return;
1206
}
1207
}
1208
+
1209
// Otherwise, commit immediately.
1232
- commitRoot(
1233
- root,
1234
- workInProgressRootRecoverableErrors,
1235
- workInProgressTransitions,
1236
- );
1210
+ commitRoot(root, recoverableErrors, transitions);
1211
}
1212
1213
function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
+4
-60
@@ -1016,49 +1016,6 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1016
expect(ReactNoop).toMatchRenderedOutput(<span prop="C" />);
1017
});
1018
1019
- // TODO: This test was written against the old Expiration Times
1020
- // implementation. It doesn't really test what it was intended to test
1021
- // anymore, because all updates to the same queue get entangled together.
1022
- // Even if they haven't expired. Consider either deleting or rewriting.
1023
- // @gate enableLegacyCache
1024
- it('flushes all expired updates in a single batch', async () => {
1025
- class Foo extends React.Component {
1026
- componentDidUpdate() {
1027
- Scheduler.log('Commit: ' + this.props.text);
1028
- }
1029
- componentDidMount() {
1030
- Scheduler.log('Commit: ' + this.props.text);
1031
- }
1032
- render() {
1033
- return (
1034
- <Suspense fallback={<Text text="Loading..." />}>
1035
- <AsyncText text={this.props.text} />
1036
- </Suspense>
1037
- );
1038
- }
1039
- }
1040
-
1041
- ReactNoop.render(<Foo text="" />);
1042
- ReactNoop.expire(1000);
1043
- jest.advanceTimersByTime(1000);
1044
- ReactNoop.render(<Foo text="go" />);
1045
- ReactNoop.expire(1000);
1046
- jest.advanceTimersByTime(1000);
1047
- ReactNoop.render(<Foo text="good" />);
1048
- ReactNoop.expire(1000);
1049
- jest.advanceTimersByTime(1000);
1050
- ReactNoop.render(<Foo text="goodbye" />);
1051
-
1052
- await waitForAll(['Suspend! [goodbye]', 'Loading...', 'Commit: goodbye']);
1053
- expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
1054
-
1055
- await resolveText('goodbye');
1056
- expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
1057
-
1058
- await waitForAll(['goodbye']);
1059
- expect(ReactNoop).toMatchRenderedOutput(<span prop="goodbye" />);
1060
- });
1061
-
1019
// @gate enableLegacyCache
1020
it('a suspended update that expires', async () => {
1021
// Regression test. This test used to fall into an infinite loop.
@@ -1780,7 +1737,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1737
});
1738
1739
// @gate enableLegacyCache
1783
- it('does suspend if a fallback has been shown for a short time', async () => {
1740
+ it('throttles content from appearing if a fallback was shown recently', async () => {
1741
function Foo() {
1742
Scheduler.log('Foo');
1743
return (
@@ -1822,23 +1779,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
1779
await resolveText('B');
1780
expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
1781
1825
- // Restart and render the complete content.
1782
+ // Restart and render the complete content. The tree will finish but we
1783
+ // won't commit the result yet because the fallback appeared recently.
1784
await waitForAll(['A', 'B']);
1827
- // TODO: Because this render was the result of a retry, and a fallback
1828
- // was shown recently, we should suspend and remain on the fallback
1829
- // for little bit longer. We currently only do this if there's still
1830
- // remaining fallbacks in the tree, but we should do it for all retries.
1831
- //
1832
- // Correct output:
1833
- // expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
1834
- //
1835
- // Actual output:
1836
- expect(ReactNoop).toMatchRenderedOutput(
1837
- <>
1838
- <span prop="A" />
1839
- <span prop="B" />
1840
- </>,
1841
- );
1785
+ expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
1786
});
1787
assertLog([]);
1788
expect(ReactNoop).toMatchRenderedOutput(