Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DevTools: Fix "unknown" updater in profiler when a component unsuspends #28330

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

DevTools: Fix "unknown" updater in profiler when a component unsuspends #28330

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9394fc
Convert ReactUpdaters to createRoot
Feb 14, 2024
dcc5ca6
DevTools: Fix "unknown" updater in profiler when a component unsuspends
eps1lon Jun 2, 2024
2abf6dc
Ignore dangling pending updaters for now
eps1lon Jun 2, 2024
eb3f84b
Avoid closure
eps1lon Jun 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion packages/react-reconciler/src/ReactFiberLane.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,27 @@ export function markRootUpdated(root: FiberRoot, updateLane: Lane) {
// idle updates until after all the regular updates have finished; there's no
// way it could unblock a transition.
if (updateLane !== IdleLane) {
if (enableUpdaterTracking) {
if (isDevToolsPresent) {
// transfer pending updaters from pingedLanes to updateLane
const pendingUpdatersLaneMap = root.pendingUpdatersLaneMap;
const updaters = pendingUpdatersLaneMap[laneToIndex(updateLane)];
let lanes = root.pingedLanes;
while (lanes > 0) {
const index = laneToIndex(lanes);
const lane = 1 << index;

const pingedUpdaters = pendingUpdatersLaneMap[index];
pingedUpdaters.forEach(pingedUpdater => {
updaters.add(pingedUpdater);
});
pingedUpdaters.clear();

lanes &= ~lane;
}
}
}

root.suspendedLanes = NoLanes;
root.pingedLanes = NoLanes;
}
Expand Down Expand Up @@ -656,7 +677,8 @@ export function markRootSuspended(
}

export function markRootPinged(root: FiberRoot, pingedLanes: Lanes) {
root.pingedLanes |= root.suspendedLanes & pingedLanes;
// TODO: When would we ever ping lanes that we aren't suspended on?
root.pingedLanes |= pingedLanes;
Comment on lines +661 to +662
Copy link
Collaborator Author

@eps1lon eps1lon Jun 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to investigate this further? Just transferring the updaters from root.pingedLanes to updateLane isn't enough. For some reason we're pinging when root.suspendedLanes & pingedLanes === NoLanes. Which seems odd intuitively but maybe the bug is root.suspendedLanes having no overlap with pingedLanes?

Copy link
Collaborator Author

@eps1lon eps1lon Jun 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would we ever ping lanes that we aren't suspended on?

Maybe if we suspend on two Wakeables A and B, A gets pinged earlier, we unsuspend completely i.e. don't suspend on B again (e.g. conditional use?) and then we wouldn't want to retry when B gets pinged?

Grasping for straws here since all tests still pass with this change.

}

export function markRootFinished(
Expand Down
17 changes: 12 additions & 5 deletions packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ describe('updaters', () => {
schedulerTags.push(fiber.tag);
schedulerTypes.push(fiber.elementType);
});
fiberRoot.pendingUpdatersLaneMap.forEach((pendingUpdaters, index) => {
if (pendingUpdaters.size > 0) {
// TODO: Is it ever ok to have dangling pending updaters or is this always a bug?
// const lane = 1 << index;
// throw new Error(
// `Lane ${lane} has pending updaters. Either you didn't assert on all updates in your test or React is leaking updaters.`,
// );
Comment on lines +54 to +58
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests that currently would fail this: https://app.circleci.com/pipelines/github/facebook/react/54155/workflows/17c8ec84-c64b-4fc1-bde6-6fcb7532513e/jobs/892710

}
});
allSchedulerTags.push(schedulerTags);
allSchedulerTypes.push(schedulerTypes);
}),
Expand Down Expand Up @@ -266,9 +275,6 @@ describe('updaters', () => {
await waitForAll([]);
});

// This test should be convertable to createRoot but the allScheduledTypes assertions are no longer the same
// So I'm leaving it in legacy mode for now and just disabling if legacy mode is turned off
// @gate !disableLegacyMode
it('should cover suspense pings', async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original issue in the test was that the updater was on the Default lane, the updater got pinged, and then we said we should render on a Retry lane (markRootPinged). This left the updater dangling in pending updaters since we only move the pending updaters to memoized on the lane we actually render (Retry lane).

let data = null;
let resolver = null;
Expand Down Expand Up @@ -303,10 +309,11 @@ describe('updaters', () => {
}
};

const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => {
ReactDOM.render(<Parent />, document.createElement('div'));
assertLog(['onCommitRoot']);
root.render(<Parent />);
});
assertLog(['onCommitRoot']);
expect(setShouldSuspend).not.toBeNull();
expect(allSchedulerTypes).toEqual([[null]]);

Expand Down