[Hydration Bugfix] Updates to dehydrated content when disableSchedulerTimeoutBasedOnReactExpirationTime is enabled
#16614
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
Adds a test case for receving an update to dehydrated content when the `disableSchedulerTimeoutBasedOnReactExpirationTime` flag is enabled.
Canceling an already running task is currently a no-op. It does not cause the task to yield execution, and if the task does yield with a continuation, then the continuation will run. This change addresses both parts: canceling the current task causes `shouldYield` to return `true`, and if the canceled task returns a continuation, the continuation is ignored. This fixes the regression test introduced in the preceding commit. There's likely a better way to model a restart of an in-progress render, but this approach is conceptually similar to how a regular high priority interruption already works.
acdlite
force-pushed
the
dehydrated-update-bugfix
branch
from
3b40cc0
to
83102b6
Compare
Details of bundled changes.Comparing: 4ef2696...83102b6 react-art
react-dom
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
trueadm
reviewed
Aug 30, 2019
| ReactFeatureFlags = require('shared/ReactFeatureFlags'); | ||
| ReactFeatureFlags.enableSuspenseServerRenderer = true; | ||
| ReactFeatureFlags.enableSuspenseCallback = true; | ||
| ReactFeatureFlags.enableFlareAPI = true; |
There was a problem hiding this comment.
Probably not necessary, I just copy pasted from the top of the file for consistency:
trueadm
reviewed
Aug 30, 2019
| @@ -288,6 +288,26 @@ describe('Scheduler', () => { | |||
| expect(Scheduler).toFlushWithoutYielding(); | |||
| }); | |||
|
|
|||
| it('cancelling the currently running task', () => { | |||
| const task = scheduleCallback(NormalPriority, () => { | |||
There was a problem hiding this comment.
Are there any other priorities we need to test cancelling on other than normal? Should there be a test to confirm that discrete priority events do not get cancelled?
There was a problem hiding this comment.
The priority shouldn't be relevant, but I'll add the same test at user-blocking priority as an extra precaution.
trueadm
approved these changes
Aug 30, 2019
There was a problem hiding this comment.
I understand the implementation here and understand what you're doing here and it looks fine for a short-term solution, as I agree – depending on the scheduler's semantics for canceling and yielding is a bit leaky.
I also added some comments in regards to some things though that you might want to respond to.
|
Superseded by #16771 |
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.
The Bug
When server rendered content that hasn't finished hydrating yet ("dehydrated" content) receives an update (via props or context), React has a mechanism to force the content to hydrate before applying the update. It does this by increasing the priority of the hydration task from Idle to a level slightly higher than the current render. React will abort the current render, perform the hydration, then try the update again on top of the now-fully-hydrated content.
There are unit tests that cover this case. The bug starts happening when
disableSchedulerTimeoutBasedOnReactExpirationTimeis enabled. It turns out that the mechanism to interrupt the current rendering task depends on the hydration task having a slightly earlier timeout, because Scheduler tasks are sorted by their timeouts. When the hydration task is higher priority, it causesshouldYieldto flip to true, forcing the render to yield execution and allowing the hydration task to start. (This is similar to how input events can interrupt normal priority renders.)disableSchedulerTimeoutBasedOnReactExpirationTimebreaks this mechanism, because when it is enabled, the timeout given to Scheduler is no longer based on React's internal expiration times. Effectively, all rendering tasks within the same priority category are first-in-first-out. So, the hydration task comes after the original task in the Scheduler queue, and thereforeshouldYieldwill keep returningfalse, and the original task will run to completion. (See #16284 for more information ondisableSchedulerTimeoutBasedOnReactExpirationTime.)The first commit in this PR adds a regression test for this case.
The Fix
There are several potential fixes. The one I've chosen is not ideal in the long term, but it's lower risk compared to the complete solution, which will likely require some refactoring of how rendering tasks are scheduled.
The work loop already has some logic to cancel a rendering task in favor of a higher priority one, using
Scheduler.cancelCallback. It does this by comparing the React expiration times of each task, so it doesn't depend on the ordering of tasks in Scheduler. This works when the high priority task is received during an input event.However,
Scheduler.cancelCallbackis currently a no-op when given an already-running task. It does not cause the task to stop execution, and if the task does yield with a continuation, then the continuation will run. Which means it won't work if React is already inside the render phase. (Note the distinction between "inside the render phase" versus "in an event that fires in between two chunks of a time sliced task.")The fix in the second commit addresses both parts: canceling the current task causes
shouldYieldto returntrue, and if the canceled task returns a continuation, the continuation is ignored.This is sufficient to fix the regression.
Alternative Fixes
A proper fix would be to model interruptions of in-progress renders in such a way that it does not depend on Scheduler's semantics for canceling and yielding. However, because of the inherent risk involved in changing how rendering tasks are scheduled, I would prefer to land this smaller fix first before attempting a refactor.
(There's already a planned mini-refactor of the work loop, e.g. to optimize how pings and restarts are modeled. We can fold this into that larger change.)