Skip to content

[Flight] Include I/O not awaited in user space #33715

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.

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

Merged
merged 2 commits into from
Jul 7, 2025

Conversation

sebmarkbage
Copy link
Collaborator

If I/O is not awaited in user space in a "previous" path we used to just drop it on the floor. There's a few strategies we could apply here. My first commit just emits it without an await but that would mean we don't have an await stack when there's no I/O in a follow up.

I went with a strategy where the "previous" I/O is used only if the "next" didn't have I/O. This may still drop I/O on the floor if there's two back to back within internals for example. It would only log the first one even though the outer await may have started earlier.

It may also log deeper in the "next" path if that had user space stacks and then the outer await will appear as if it awaited after.

So it's not perfect.

@sebmarkbage sebmarkbage requested a review from eps1lon July 6, 2025 21:28
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Jul 6, 2025
@react-sizebot
Copy link

Comparing: 9a645e1...28b7f3e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 530.50 kB 530.50 kB = 93.66 kB 93.66 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 655.04 kB 655.04 kB = 115.35 kB 115.35 kB
facebook-www/ReactDOM-prod.classic.js = 675.12 kB 675.12 kB = 118.77 kB 118.77 kB
facebook-www/ReactDOM-prod.modern.js = 665.54 kB 665.54 kB = 117.13 kB 117.13 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 28b7f3e

}
case PROMISE_NODE: {
if (node.end <= request.timeOrigin) {
// This was already resolved when we started this render. It must have been either something
// that's part of a start up sequence or externally cached data. We exclude that information.
// The technique for debugging the effects of uncached data on the render is to simply uncache it.
return null;
return previousIONode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment above makes we wonder whether this change may affect cached I/O. At least in the Flight fixture it doesn't seem to cause an issue with the cached third-party stream though.

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 previousIONode has its own check if it finished before this sequence so it’ll also be null.

We don’t really expect the future node to have finished before the previous. But for consistency I return it here.

@sebmarkbage sebmarkbage merged commit 0378b46 into facebook:main Jul 7, 2025
247 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants