Skip to content

Commit 8a02664

Browse files
acdliteacdlite
committed
Bugfix: Nested useOpaqueIdentifier calls
Fixes an issue where multiple useOpaqueIdentifier hooks are upgraded to client ids within the same render. The way the upgrade works is that useOpaqueIdentifier schedules a render phase update then throws an error to trigger React's error recovery mechanism. The normal error recovery mechanism is designed for errors that occur as a result of interleaved mutations, so we usually only retry a single time, synchronously, before giving up. useOpaqueIdentifier is different because the error its throws when upgrading is not caused by an interleaved mutation. Rather, it happens when an ID is referenced for the first time inside a client-rendered tree (i.e. sommething that wasn't part of the initial server render). The fact that it relies on the error recovery mechanism is an implementation detail. And a single recovery attempt may be insufficient. For example, if a parent and a child component may reference different ids, and both are mounted as a result of the same client update, that will trigger two separate error recovery attempts. Because render phase updates are not allowed when triggered from userspace — we log a warning in developement to prevent them — we can assume that if something does update during the render phase, it is one of our "legit" implementation details like useOpaqueIdentifier. So we can keep retrying until we succeed — up to a limit, to protect against inifite loops. I chose 50 since that's the limit we use for commit phase updates.
1 parent c71d3ab commit 8a02664

File tree

4 files changed

+168
-14
lines changed

4 files changed

+168
-14
lines changed

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1975,5 +1975,40 @@ describe('ReactDOMServerHooks', () => {
19751975
container.getElementsByTagName('span')[0].getAttribute('id'),
19761976
).not.toBeNull();
19771977
});
1978+
1979+
it('useOpaqueIdentifier with multiple ids in nested components', async () => {
1980+
function DivWithId({id, children}) {
1981+
return <div id={id}>{children}</div>;
1982+
}
1983+
1984+
let setShowMore;
1985+
function App() {
1986+
const outerId = useOpaqueIdentifier();
1987+
const innerId = useOpaqueIdentifier();
1988+
const [showMore, _setShowMore] = useState(false);
1989+
setShowMore = _setShowMore;
1990+
return showMore ? (
1991+
<DivWithId id={outerId}>
1992+
<DivWithId id={innerId} />
1993+
</DivWithId>
1994+
) : null;
1995+
}
1996+
1997+
const container = document.createElement('div');
1998+
container.innerHTML = ReactDOMServer.renderToString(<App />);
1999+
2000+
await act(async () => {
2001+
ReactDOM.hydrateRoot(container, <App />);
2002+
});
2003+
2004+
// Show additional content that wasn't part of the initial server-
2005+
// rendered repsonse.
2006+
await act(async () => {
2007+
setShowMore(true);
2008+
});
2009+
const [div1, div2] = container.getElementsByTagName('div');
2010+
expect(typeof div1.getAttribute('id')).toBe('string');
2011+
expect(typeof div2.getAttribute('id')).toBe('string');
2012+
});
19782013
});
19792014
});

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

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,9 @@ let workInProgressRootIncludedLanes: Lanes = NoLanes;
294294
// includes unprocessed updates, not work in bailed out children.
295295
let workInProgressRootSkippedLanes: Lanes = NoLanes;
296296
// Lanes that were updated (in an interleaved event) during this render.
297-
let workInProgressRootUpdatedLanes: Lanes = NoLanes;
297+
let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
298+
// Lanes that were updated during the render phase (*not* an interleaved event).
299+
let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
298300
// Lanes that were pinged (in an interleaved event) during this render.
299301
let workInProgressRootPingedLanes: Lanes = NoLanes;
300302

@@ -474,6 +476,12 @@ export function scheduleUpdateOnFiber(
474476
// an implementation detail, like selective hydration
475477
// and useOpaqueIdentifier.
476478
warnAboutRenderPhaseUpdatesInDEV(fiber);
479+
480+
// Track lanes that were updated during the render phase
481+
workInProgressRootRenderPhaseUpdatedLanes = mergeLanes(
482+
workInProgressRootRenderPhaseUpdatedLanes,
483+
lane,
484+
);
477485
} else {
478486
// This is a normal update, scheduled from outside the render phase. For
479487
// example, during an input event.
@@ -514,8 +522,8 @@ export function scheduleUpdateOnFiber(
514522
deferRenderPhaseUpdateToNextBatch ||
515523
(executionContext & RenderContext) === NoContext
516524
) {
517-
workInProgressRootUpdatedLanes = mergeLanes(
518-
workInProgressRootUpdatedLanes,
525+
workInProgressRootInterleavedUpdatedLanes = mergeLanes(
526+
workInProgressRootInterleavedUpdatedLanes,
519527
lane,
520528
);
521529
}
@@ -878,7 +886,25 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
878886
clearContainer(root.containerInfo);
879887
}
880888

881-
const exitStatus = renderRootSync(root, errorRetryLanes);
889+
let exitStatus;
890+
891+
const MAX_ERROR_RETRY_ATTEMPTS = 50;
892+
for (let i = 0; i < MAX_ERROR_RETRY_ATTEMPTS; i++) {
893+
exitStatus = renderRootSync(root, errorRetryLanes);
894+
if (
895+
exitStatus === RootErrored &&
896+
workInProgressRootRenderPhaseUpdatedLanes !== NoLanes
897+
) {
898+
// There was a render phase update during this render. This was likely a
899+
// useOpaqueIdentifier hook upgrading itself to a client ID. Try rendering
900+
// again. This time, the component will use a client ID and will proceed
901+
// without throwing. If multiple IDs upgrade as a result of the same
902+
// update, we will have to do multiple render passes. To protect against
903+
// an inifinite loop, eventually we'll give up.
904+
continue;
905+
}
906+
break;
907+
}
882908

883909
executionContext = prevExecutionContext;
884910

@@ -1055,7 +1081,10 @@ function markRootSuspended(root, suspendedLanes) {
10551081
// TODO: Lol maybe there's a better way to factor this besides this
10561082
// obnoxiously named function :)
10571083
suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);
1058-
suspendedLanes = removeLanes(suspendedLanes, workInProgressRootUpdatedLanes);
1084+
suspendedLanes = removeLanes(
1085+
suspendedLanes,
1086+
workInProgressRootInterleavedUpdatedLanes,
1087+
);
10591088
markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes);
10601089
}
10611090

@@ -1313,7 +1342,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
13131342
workInProgressRootExitStatus = RootIncomplete;
13141343
workInProgressRootFatalError = null;
13151344
workInProgressRootSkippedLanes = NoLanes;
1316-
workInProgressRootUpdatedLanes = NoLanes;
1345+
workInProgressRootInterleavedUpdatedLanes = NoLanes;
1346+
workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
13171347
workInProgressRootPingedLanes = NoLanes;
13181348

13191349
enqueueInterleavedUpdates();
@@ -1456,7 +1486,7 @@ export function renderDidSuspendDelayIfPossible(): void {
14561486
if (
14571487
workInProgressRoot !== null &&
14581488
(includesNonIdleWork(workInProgressRootSkippedLanes) ||
1459-
includesNonIdleWork(workInProgressRootUpdatedLanes))
1489+
includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes))
14601490
) {
14611491
// Mark the current render as suspended so that we switch to working on
14621492
// the updates that were skipped. Usually we only suspend at the end of

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

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,9 @@ let workInProgressRootIncludedLanes: Lanes = NoLanes;
294294
// includes unprocessed updates, not work in bailed out children.
295295
let workInProgressRootSkippedLanes: Lanes = NoLanes;
296296
// Lanes that were updated (in an interleaved event) during this render.
297-
let workInProgressRootUpdatedLanes: Lanes = NoLanes;
297+
let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
298+
// Lanes that were updated during the render phase (*not* an interleaved event).
299+
let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
298300
// Lanes that were pinged (in an interleaved event) during this render.
299301
let workInProgressRootPingedLanes: Lanes = NoLanes;
300302

@@ -474,6 +476,12 @@ export function scheduleUpdateOnFiber(
474476
// an implementation detail, like selective hydration
475477
// and useOpaqueIdentifier.
476478
warnAboutRenderPhaseUpdatesInDEV(fiber);
479+
480+
// Track lanes that were updated during the render phase
481+
workInProgressRootRenderPhaseUpdatedLanes = mergeLanes(
482+
workInProgressRootRenderPhaseUpdatedLanes,
483+
lane,
484+
);
477485
} else {
478486
// This is a normal update, scheduled from outside the render phase. For
479487
// example, during an input event.
@@ -514,8 +522,8 @@ export function scheduleUpdateOnFiber(
514522
deferRenderPhaseUpdateToNextBatch ||
515523
(executionContext & RenderContext) === NoContext
516524
) {
517-
workInProgressRootUpdatedLanes = mergeLanes(
518-
workInProgressRootUpdatedLanes,
525+
workInProgressRootInterleavedUpdatedLanes = mergeLanes(
526+
workInProgressRootInterleavedUpdatedLanes,
519527
lane,
520528
);
521529
}
@@ -878,7 +886,25 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
878886
clearContainer(root.containerInfo);
879887
}
880888

881-
const exitStatus = renderRootSync(root, errorRetryLanes);
889+
let exitStatus;
890+
891+
const MAX_ERROR_RETRY_ATTEMPTS = 50;
892+
for (let i = 0; i < MAX_ERROR_RETRY_ATTEMPTS; i++) {
893+
exitStatus = renderRootSync(root, errorRetryLanes);
894+
if (
895+
exitStatus === RootErrored &&
896+
workInProgressRootRenderPhaseUpdatedLanes !== NoLanes
897+
) {
898+
// There was a render phase update during this render. This was likely a
899+
// useOpaqueIdentifier hook upgrading itself to a client ID. Try rendering
900+
// again. This time, the component will use a client ID and will proceed
901+
// without throwing. If multiple IDs upgrade as a result of the same
902+
// update, we will have to do multiple render passes. To protect against
903+
// an inifinite loop, eventually we'll give up.
904+
continue;
905+
}
906+
break;
907+
}
882908

883909
executionContext = prevExecutionContext;
884910

@@ -1055,7 +1081,10 @@ function markRootSuspended(root, suspendedLanes) {
10551081
// TODO: Lol maybe there's a better way to factor this besides this
10561082
// obnoxiously named function :)
10571083
suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);
1058-
suspendedLanes = removeLanes(suspendedLanes, workInProgressRootUpdatedLanes);
1084+
suspendedLanes = removeLanes(
1085+
suspendedLanes,
1086+
workInProgressRootInterleavedUpdatedLanes,
1087+
);
10591088
markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes);
10601089
}
10611090

@@ -1313,7 +1342,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
13131342
workInProgressRootExitStatus = RootIncomplete;
13141343
workInProgressRootFatalError = null;
13151344
workInProgressRootSkippedLanes = NoLanes;
1316-
workInProgressRootUpdatedLanes = NoLanes;
1345+
workInProgressRootInterleavedUpdatedLanes = NoLanes;
1346+
workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
13171347
workInProgressRootPingedLanes = NoLanes;
13181348

13191349
enqueueInterleavedUpdates();
@@ -1456,7 +1486,7 @@ export function renderDidSuspendDelayIfPossible(): void {
14561486
if (
14571487
workInProgressRoot !== null &&
14581488
(includesNonIdleWork(workInProgressRootSkippedLanes) ||
1459-
includesNonIdleWork(workInProgressRootUpdatedLanes))
1489+
includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes))
14601490
) {
14611491
// Mark the current render as suspended so that we switch to working on
14621492
// the updates that were skipped. Usually we only suspend at the end of

‎packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1915,6 +1915,65 @@ describe('ReactIncrementalErrorHandling', () => {
19151915
expect(root).toMatchRenderedOutput('Everything is fine.');
19161916
});
19171917

1918+
it("does not infinite loop if there's a render phase update in the same render as an error", async () => {
1919+
// useOpaqueIdentifier uses an render phase update as an implementation
1920+
// detail. When an error is accompanied by a render phase update, we assume
1921+
// that it comes from useOpaqueIdentifier, because render phase updates
1922+
// triggered from userspace are not allowed (we log a warning). So we keep
1923+
// attempting to recover until no more opaque identifiers need to be
1924+
// upgraded. However, we should give up after some point to prevent an
1925+
// infinite loop in the case where there is (by accident) a render phase
1926+
// triggered from userspace.
1927+
1928+
spyOnDev(console, 'error');
1929+
1930+
let numberOfThrows = 0;
1931+
1932+
let setStateInRenderPhase;
1933+
function Child() {
1934+
const [, setState] = React.useState(0);
1935+
setStateInRenderPhase = setState;
1936+
return 'All good';
1937+
}
1938+
1939+
function App({shouldThrow}) {
1940+
if (shouldThrow) {
1941+
setStateInRenderPhase();
1942+
numberOfThrows++;
1943+
throw new Error('Oops!');
1944+
}
1945+
return <Child />;
1946+
}
1947+
1948+
const root = ReactNoop.createRoot();
1949+
await act(async () => {
1950+
root.render(<App shouldThrow={false} />);
1951+
});
1952+
expect(root).toMatchRenderedOutput('All good');
1953+
1954+
let error;
1955+
try {
1956+
await act(async () => {
1957+
root.render(<App shouldThrow={true} />);
1958+
});
1959+
} catch (e) {
1960+
error = e;
1961+
}
1962+
1963+
expect(error.message).toBe('Oops!');
1964+
expect(numberOfThrows < 100).toBe(true);
1965+
1966+
if (__DEV__) {
1967+
expect(console.error).toHaveBeenCalledTimes(2);
1968+
expect(console.error.calls.argsFor(0)[0]).toContain(
1969+
'Cannot update a component (`%s`) while rendering a different component',
1970+
);
1971+
expect(console.error.calls.argsFor(1)[0]).toContain(
1972+
'The above error occurred in the <App> component',
1973+
);
1974+
}
1975+
});
1976+
19181977
if (global.__PERSISTENT__) {
19191978
it('regression test: should fatal if error is thrown at the root', () => {
19201979
const root = ReactNoop.createRoot();

0 commit comments

Comments
 (0)