Bugfix: Expired partial tree infinite loops #17949
Merged
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
facebook-github-bot
added
CLA Signed
React Core Team
Details of bundled changes.Comparing: d2158d6...7ce5183 react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
Size changes (stable) |
Details of bundled changes.Comparing: d2158d6...7ce5183 react-art
react-native-renderer
react-dom
react-test-renderer
react-reconciler
Size changes (experimental) |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7ce5183:
|
acdlite
changed the title
acdlite
force-pushed
the
regression-expired-shouldyield
branch
2 times, most recently
from
74d172b
to
51ef007
Compare
sebmarkbage
reviewed
Feb 4, 2020
acdlite
force-pushed
the
regression-expired-shouldyield
branch
2 times, most recently
from
38c38b6
to
010c09b
Compare
|
@sebmarkbage Based on your feedback I updated the PR in two commits. The first one moves the partial tree check to The second one removes |
acdlite
force-pushed
the
regression-expired-shouldyield
branch
from
acb551e
to
b074e6d
Compare
sebmarkbage
approved these changes
Feb 11, 2020
| @@ -1493,6 +1440,53 @@ function workLoopSync() { | |||
| } | |||
| } | |||
|
|
|||
| function renderRootConcurrent(root, expirationTime) { | |||
There was a problem hiding this comment.
Just to be clear, this is only called once so it didn't technically need to be extracted out, right?
There was a problem hiding this comment.
Yeah I only did it for symmetry purposes
acdlite
force-pushed
the
regression-expired-shouldyield
branch
2 times, most recently
from
849a102
to
9fb1194
Compare
* Failing test: Expiring a partially completed tree We should not throw out a partially completed tree if it expires in the middle of rendering. We should finish the rest of the tree without yielding, then finish any remaining expired levels in a single batch. * Check if there's a partial tree before restarting If a partial render expires, we should stay in the concurrent path (performConcurrentWorkOnRoot); we'll stop yielding, but the rest of the behavior remains the same. We will only revert to the sync path (performSyncWorkOnRoot) when starting on a new level. This approach prevents partially completed concurrent work from being discarded. * New test: retry after error during expired render
Adds regression tests that reproduce a scenario where a partially completed tree expired then fell into an infinite loop. The code change that exposed this bug made the assumption that if you call Scheduler's `shouldYield` from inside an expired task, Scheduler will always return `false`. But in fact, Scheduler sometimes returns `true` in that scenario, which is a bug. The reason it worked before is that once a task timed out, React would always switch to a synchronous work loop without checking `shouldYield`. My rationale for relying on `shouldYield` was to unify the code paths between a partially concurrent render (i.e. expires midway through) and a fully concurrent render, as opposed to a render that was synchronous the whole time. However, this bug indicates that we need a stronger guarantee within React for when tasks expire, given that the failure case is so catastrophic. Instead of relying on the result of a dynamic method call, we should use control flow to guarantee that the work is synchronously executed. (We should also fix the Scheduler bug so that `shouldYield` always returns false inside an expired task, but I'll address that separately.)
Refactors the `didTimeout` check so that it always switches to the synchronous work loop, like it did before the regression. This breaks the error handling behavior that I added in 5f7361f (an error during a partially concurrent render should retry once, synchronously). I'll fix this next. I need to change that behavior, anyway, to support retries that occur as a result of `flushSync`.
Except in legacy mode. This is to support the `useOpaqueReference` hook, which uses an error to trigger a retry at lower priority.
Splits the work loop and its surrounding enter/exit code into their own functions. Now we can do perform multiple render phase passes within a single call to performConcurrentWorkOnRoot or performSyncWorkOnRoot. This lets us get rid of the `didError` field.
acdlite
force-pushed
the
regression-expired-shouldyield
branch
from
9fb1194
to
7ce5183
Compare
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.
Adds regression tests that reproduce a scenario where a partially completed tree expired then fell into an infinite loop.
The code change that exposed this bug made the assumption that if you call Scheduler's
shouldYieldfrom inside an expired task, Scheduler will always returnfalse. But in fact, Scheduler sometimes returnstruein that scenario, which is a bug.The reason it worked before is that once a task timed out, React would always switch to a synchronous work loop without checking
shouldYield.My rationale for relying on
shouldYieldwas to unify the code paths between a partially concurrent render (i.e. expires midway through) and a fully concurrent render, as opposed to a render that was synchronous the whole time. However, this bug indicates that we need a stronger guarantee within React for when tasks expire, given that the failure case is so catastrophic. Instead of relying on the result of a dynamic method call, we should use control flow to guarantee that the work is synchronously executed.(We should also fix the Scheduler bug so that
shouldYieldalways returns false inside an expired task, but I'll address that separately.)