Skip to content

Commit

Permalink
Bugfix: Expired partial tree infinite loops (#17949)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
acdlite committed Mar 3, 2020
1 parent d2158d6 commit ec652f4
Show file tree
Hide file tree
Showing 5 changed files with 494 additions and 153 deletions.
Loading

0 comments on commit ec652f4

Please sign in to comment.