Move renderDidSuspend logic to throwException #25852
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I found this bug when working on a different task. `pingSuspendedRoot` sometimes calls `prepareFreshStack` to interupt the work-in-progress tree and force a restart from the root. The idea is that if the current render is already in a state where it be blocked from committing, and there's new data that could unblock it, we might as well restart from the beginning. The problem is that this is only safe to do if `pingSuspendedRoot` is called from a non-React task, like an event handler or a microtask. While this is usually the case, it's entirely possible for a thenable to resolve (i.e. to call `pingSuspendedRoot`) synchronously while the render phase is already executing. If that happens, and work loop attempts to unwind the stack, it causes the render phase to crash. This commit adds a regression test that reproduces one of these scenarios.
This is part of a larger refactor I'm doing to simplify how this logic works, since it's currently spread out and duplicated across several different parts of the work loop. When a Suspense boundary shows a fallback, and it wasn't already showing a fallback, we mark the render as suspended. Knowing whether the in-progress tree includes a new fallback is useful for several reasons: we can choose to interrupt the current render and switch to a different task, we can suspend the work loop until more data becomes available, we can throttle the appearance of multiple fallback states, and so on. Currently this logic happens in the complete phase, but because we use renderDidSuspendWithDelay as a signal to interrupt the work loop, it's best to do it early as we possibly can. A subsequent step will move the logic even earlier, as soon as we attempt to unwrap an unresolved promise, so that `use` can determine whether it's OK to pause the entire work loop and wait for the promise. There's some existing code that attempts to do this but it doesn't have parity with how `renderDidSuspend` works, which is the main motivation for this series of refactors.
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 7c39922...ca09d0c Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
|
Landed as part of #25922 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on bugfix in #25851
This is part of a larger refactor I'm doing to simplify how this logic works, since it's currently spread out and duplicated across several different parts of the work loop.
When a Suspense boundary shows a fallback, and it wasn't already showing a fallback, we mark the render as suspended. Knowing whether the in-progress tree includes a new fallback is useful for several reasons: we can choose to interrupt the current render and switch to a different task, we can suspend the work loop until more data becomes available, we can throttle the appearance of multiple fallback states, and so on.
Currently this logic happens in the complete phase, but because we use renderDidSuspendWithDelay as a signal to interrupt the work loop, it's best to do it early as we possibly can.
A subsequent step will move the logic even earlier, as soon as we attempt to unwrap an unresolved promise, so that
usecan determine whether it's OK to pause the entire work loop and wait for the promise. There's some existing code that attempts to do this but it doesn't have parity with howrenderDidSuspendworks, which is the main motivation for this series of refactors.