-
Notifications
You must be signed in to change notification settings - Fork 49.7k
[Fizz] Push halted await to the owner stack for late-arriving I/O info #35019
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
[Fizz] Push halted await to the owner stack for late-arriving I/O info #35019
Conversation
To quote from facebook#33634: > If an aborted task is not rendering, then this is an async abort. > Conceptually it's as if the abort happened inside the async gap. The > abort reason's stack frame won't have that on the stack so instead we > use the owner stack and debug task of any halted async debug info. This PR extends that logic to also try to resolve lazy components to find debug info that has been transferred to the inner value. In addition, we ignore any time and component info that might precede the I/O info, effectively allowing resolved I/O to also be considered for the owner stack. This is useful in a scenario where the Flight rendering might have been completed (and not prematurely aborted), but then the Fizz rendering is intentionally aborted before all chunks were received, while still allowing the remaining chunks (including I/O info for halted components) to be processed while the prerender is in the aborting state.
5d467bc to
64c896b
Compare
| if (typeof info.time === 'number') { | ||
| // This had an end time. Any awaits before this must have already resolved. | ||
| break; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit is not right. We can't just take the last entry that's resolved since that doesn't tell us about where it was aborted.
We need another solution to this (e.g. an end time in the flight client that avoids adding debug info after some time stamp).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're picking the first resolved entry. (I at least intended to do so, needs to be fixed!) My thinking was that this should point at the location where the I/O would have halted the component if we didn't render through fully. Any cached I/O that was awaited before wouldn't be emitted as I/O, so we shouldn't point at those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed your proposal with @eps1lon. Makes sense now, will implement that.
Not really happy about calling the public option `endTime` instead of `debugEndTime`, but this is following the `startTime`/`debugStartTime` naming pattern.
That's too late though. We probably need to prevent adding the debug info in the first place.
This reverts commit 665c671b0b87640d1fc52c39eb4aec4f5b0c72ee.
45bfa94 to
bd8ae71
Compare
To quote from #33634:
This PR extends that logic to also try to resolve lazy components to find debug info that has been transferred to the inner value (same approach as in #34839).
In addition, we now ignore any time and component info that might follow the I/O info, effectively allowing resolved I/O to also be considered for the owner stack. This is useful in a scenario where the Flight rendering might have been completed (and not prematurely aborted), but then the Fizz rendering is intentionally aborted before all chunks were received, while still allowing the remaining chunks (including I/O info for halted components) to be processed while the prerender is in the aborting state.
An
endTimeoption can be passed to the Flight client to indicate that all debug info that arrives after this time should be ignored. When rendering in Fizz is then aborted, the late-arriving debug info that's used to enhance the owner stack only includes I/O info up to that end time.