Skip to content

Commit ec652f4

Browse files
acdliteacdlite
authored
Bugfix: Expired partial tree infinite loops (#17949)
* Bugfix: Expiring a partially completed tree (#17926) * 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 * Regression: Expired partial tree infinite loops 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.) * Always switch to sync work loop when task expires 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`. * Retry once after error even for sync renders Except in legacy mode. This is to support the `useOpaqueReference` hook, which uses an error to trigger a retry at lower priority. * Move partial tree check to performSyncWorkOnRoot * Factor out render phase 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.
1 parent d2158d6 commit ec652f4

File tree

5 files changed

+494
-153
lines changed

5 files changed

+494
-153
lines changed

0 commit comments

Comments
 (0)