Skip to content
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

[New Scheduler] Fix: Suspending an expired update #15326

Merged
merged 1 commit into from Apr 4, 2019

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 4, 2019

When an async update expires, React renders at the expiration time that corresponds to the current time, not at the original update's expiration time. That way, all the expired work in the tree is flushed in a single batch.

This is implemented inside renderRoot by comparing the next render expiration time to the current time. If the current time is later, renderRoot will restart at the later time.

Because of poor factoring, the check is currently performed right before entering the work loop. But the work loop is usually entered multiple times in a single callback: each time a component throws or suspends. This led to an infinite loop where React would detect that an update expired, restart at the current time, make a bit of progress, suspend, check for expired work again, and start the loop again.

I fixed this by moving the expired work check to the beginning of renderRoot, so that it is not performed every time something suspends. This isn't ideal, because you could technically still fall into a loop if more than 10ms lapse in between exiting renderRoot and entering it again. The proper fix is to lift the check outside of renderRoot entirely so that the function can restart without checking for expired work again. Since this is exceedingly unlikely (and this whole thing is still behind a flag), I'll do the better fix in an already-planned follow up to fork renderRoot into separate functions for sync and async work.

When an async update expires, React renders at the expiration time that
corresponds to the current time, not at the original update's expiration
time. That way, all the expired work in the tree is flushed in a
single batch.

This is implemented inside `renderRoot` by comparing the next render
expiration time to the current time. If the current time is later,
`renderRoot` will restart at the later time.

Because of poor factoring, the check is currently performed right before
entering the work loop. But the work loop is usually entered multiple
times in a single callback: each time a component throws or suspends.
This led to an infinite loop where React would detect that an update
expired, restart at the current time, make a bit of progress, suspend,
check for expired work again, and start the loop again.

I fixed this by moving the expired work check to the beginning of
`renderRoot`, so that it is not performed every time something suspends.
This isn't ideal, because you could technically still fall into a loop
if more than 10ms lapse in between exiting `renderRoot` and entering it
again. The proper fix is to lift the check outside of `renderRoot`
entirely so that the function can restart without checking for expired
work again. Since this is exceedingly unlikely (and this whole thing is
still behind a flag), I'll do the better fix in an already-planned
follow up to fork `renderRoot` into separate functions for sync and
async work.
@acdlite acdlite merged commit 49595e9 into facebook:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants