[Flight] Track Debug Info from Synchronously Unwrapped Promises #33485
Conversation
github-actions
bot
added
the
React Core Team
| // This is a temporary fork of the `use` implementation until we accept | ||
| // promises everywhere. | ||
| const thenable: Thenable<mixed> = (wakeable: any); | ||
| switch (thenable.status) { | ||
| case 'fulfilled': | ||
| case 'fulfilled': { | ||
| forwardDebugInfoFromThenable(request, task, thenable); |
There was a problem hiding this comment.
This only applies to the internal temporary wrapper of Flight.
With this approach the Promise inside React.lazy is never tracked since it disappears and it behaves more like the throwing-a-promise approach. The idea is to convert these to just return Promises eventually though.
You can still assign manual _debugInfo to a lazy though.
However, we also only track Lazy in the child position. Lazy element.type (the only documented approach) are not tracked. So if you have some I/O to load some module with a Component in it, that wouldn't be tracked atm.
| forwardDebugInfo(request, newTask, debugInfo); | ||
| } | ||
| } | ||
| forwardDebugInfoFromCurrentContext(request, newTask, thenable); |
There was a problem hiding this comment.
We should just call forwardDebugInfoFromPromise here. The difference is that if this is not actually a native Promise but is resolved by a thenable which is in turn resolved by I/O (like the test in AsyncDebugInfo-test) or a native Promise then we can pick it up from the context it is resolved in.
The problem is just that now it's inconsistent because such as Thenable will only forward the information if it wasn't already resolved. If it was sync, the information would get lost.
So this is a bit inconsistent about when we expect Thenables to be manually instrumented with debugInfo like the ones coming out of Flight and when we can pick it up automatically.
The ones from FlightClient are also inconsistent in that they may pick up the I/O of the stream that loaded them when they weren't sync but if they were sync then only the forwarded debugInfo from the other server is included.
sebmarkbage
force-pushed
the
iosync
branch
3 times, most recently
from
dc02618 to
06d85b7
Compare
This is tricky because there can be parallel visits and really we should probably generate a list, sort and then emit the sorted list.
Only count something as contributing to the children end time if it was actually logged This excludes timing of promises that we didn't log an await for such as microtasks. Avoids bumping too many parallel levels.
promiseResolve, for already resolved promises, is invoked at the end of its execution phase. That means that any awaits that happen inside would have their start time below the end time which cuts them off.
``` -1..........-2.....------ ---3....................- ```
advanceTaskTime should be used before an operation to advance time before emitting that operation. markOperationEndTime should be used as its own marker after an operation. This needs to always be emitted even if it doesn't advance time.
An async function's last piece of code doesn't have any way to trace it back to the last await it was waiting for inside the body. Instead we guess this from whatever executed last.
…itted This lets us include zero width entries for parallel things that weren't blocking but might still be interesting.
We visit all the previous nodes before making any decisions about what to include so we potentially visit a lot of nodes over and over. We can optimize a bit by just stopping for anything that finished before the request start.
Unfortunately there's no public API to get to the internal symbols but AsyncResource is conceptually the same class so we can repurpose its method that does the same thing.
So might look for it on thenables when passed to use().
Before we wrap it in a Lazy since it won't be picked up by the lazy.
If we're forwarding debugInfo and also visiting the underlying native Promise, make sure we're not emitting the same debugInfo twice.
This will be used to look at the diff to the next commit
The owner/stack disappears in this case because the use() is not a real await.
Then pass this as the default if there is no deeper await. This means that use() behaves similar to await.
Stacked on #33482.
There's a flaw with getting information from the execution context of the ping. For the soft-deprecated "throw a promise" technique, this is a bit unreliable because you could in theory throw the same one multiple times. Similarly, a more fundamental flaw with that API is that it doesn't allow for tracking the information of Promises that are already synchronously able to resolve.
This stops tracking the async debug info in the case of throwing a Promise and only when you render a Promise. That means some loss of data but we should just warn for throwing a Promise anyway.
Instead, this also adds support for tracking
use()d thenables and forwarding_debugInfofrom then. This is done by extracting the info from the Promise after the fact instead of in the resolve so that it only happens once at the end after the pings are done.This also supports passing the same Promise in multiple places and tracking the debug info at each location, even if it was already instrumented with a synchronous value by the time of the second use.