[Flight] Encode Async I/O Tasks using the Enclosing Line/Column #33403
Conversation
github-actions
bot
added
the
React Core Team
sebmarkbage
force-pushed
the
ioenclosinglinecol
branch
from
9bccfae to
020ce0f
Compare
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
|
@CodiumAI-Agent /describe |
Title[Flight] Encode Async I/O Tasks using the Enclosing Line/Column User descriptionStacked on #33402. There's a bug in Chrome Performance tracking which uses the enclosing line/column instead of the callsite in stacks. For our fake eval:ed functions that represents functions on the server, we can position the enclosing function body at the position of the callsite to simulate getting the right line. Unfortunately, that doesn't give us exactly the right callsite when it's used for other purposes that uses the callsite like console logs and error reporting and stacks inside breakpoints. So I don't think we want to always do this. For ReactAsyncInfo/ReactIOInfo, the only thing we're going to use the fake task for is the Performance tracking, so it doesn't have any downsides until Chrome fixes the bug and we'd have to revert it. Therefore this PR uses that techniques only for those entries. We could do this for Server Components too but we're going to use those for other things too like console logs. I don't think it's worth duplicating the Task objects. That would also make it inconsistent with Client Components. For Client Components, we could in theory also generate fake evals but that would be way slower since there's so many of them and currently we rely on the native implementation for those. So doesn't seem worth fixing. But since we can at least fix it for RSC I/O/awaits we can do this hack. PR TypeEnhancement, Tests Description
Changes walkthrough 📝
|
|
@CodiumAI-Agent /review |
sebmarkbage
force-pushed
the
ioenclosinglinecol
branch
from
020ce0f to
c9ef691
Compare
Stacked on #33403. When a Promise is coming from React such as when it's passed from another environment, we should forward the debug information from that environment. We already do that when rendered as a child. This makes it possible to also `await promise` and have the information from that instrumented promise carry through to the next render. This is a bit tricky because the current protocol is that we have to read it from the Promise after it resolves so it has time to be assigned to the promise. `async_hooks` doesn't pass us the instance (even though it has it) when it gets resolved so we need to keep it around. However, we have to be very careful because if we get this wrong it'll cause a memory leak since we retain things by `asyncId` and then manually listen for `destroy()` which can only be called once a Promise is GC:ed, which it can't be if we retain it. We have to therefore use a `WeakRef` in case it never resolves, and then read the `_debugInfo` when it resolves. We could maybe install a setter or something instead but that's also heavy. The other issues is that we don't use native Promises in ReactFlightClient so our instrumented promises aren't picked up by the `async_hooks` implementation and so we never get a handle to our thenable instance. To solve this we can create a native wrapper only in DEV.
| useEnclosingLine ? line : enclosingLine, | ||
| useEnclosingLine ? col : enclosingCol, |
There was a problem hiding this comment.
Looking at these two lines in isolation, it seems that the condition should be flipped. Am I missing context, or is this wrong?
There was a problem hiding this comment.
I think I get it now, if useEnclosingLine/Col is true, we set the line/col also as enclosing line/col, so that it's not off when Chrome picks the enclosing line/col. I guess the name of the boolean variable confused me a bit.
Stacked on #33402.
There's a bug in Chrome Performance tracking which uses the enclosing line/column instead of the callsite in stacks.
For our fake eval:ed functions that represents functions on the server, we can position the enclosing function body at the position of the callsite to simulate getting the right line.
Unfortunately, that doesn't give us exactly the right callsite when it's used for other purposes that uses the callsite like console logs and error reporting and stacks inside breakpoints. So I don't think we want to always do this.
For ReactAsyncInfo/ReactIOInfo, the only thing we're going to use the fake task for is the Performance tracking, so it doesn't have any downsides until Chrome fixes the bug and we'd have to revert it. Therefore this PR uses that techniques only for those entries.
We could do this for Server Components too but we're going to use those for other things too like console logs. I don't think it's worth duplicating the Task objects. That would also make it inconsistent with Client Components.
For Client Components, we could in theory also generate fake evals but that would be way slower since there's so many of them and currently we rely on the native implementation for those. So doesn't seem worth fixing.
But since we can at least fix it for RSC I/O/awaits we can do this hack.