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

Bugfix: Expiring a partially completed tree #17926

Merged
merged 3 commits into from
Jan 29, 2020

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jan 29, 2020

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.

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.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 29, 2020
@sizebot
Copy link

sizebot commented Jan 29, 2020

Details of bundled changes.

Comparing: 241c446...5f7361f

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-dev.js +0.1% +0.1% 741.62 KB 742.08 KB 155.72 KB 155.86 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% 0.0% 266.8 KB 266.82 KB 45.77 KB 45.77 KB RN_OSS_PROD
ReactNativeRenderer-dev.js +0.1% +0.1% 751 KB 751.46 KB 157.93 KB 158.07 KB RN_OSS_DEV
ReactFabric-profiling.js 0.0% 0.0% 277.96 KB 277.98 KB 47.89 KB 47.9 KB RN_OSS_PROFILING
ReactNativeRenderer-prod.js 0.0% 0.0% 274.37 KB 274.4 KB 47.02 KB 47.03 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 285.58 KB 285.61 KB 49.17 KB 49.18 KB RN_OSS_PROFILING

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.production.min.js 0.0% 0.0% 72.52 KB 72.53 KB 21.31 KB 21.32 KB NODE_PROD
react-reconciler.development.js +0.1% +0.1% 609.87 KB 610.33 KB 127.71 KB 127.84 KB NODE_DEV
react-reconciler.production.min.js 0.0% 0.0% 72.51 KB 72.52 KB 21.31 KB 21.31 KB NODE_PROD
react-reconciler-reflection.development.js 0.0% -0.0% 19.51 KB 19.51 KB 6.39 KB 6.39 KB NODE_DEV
react-reconciler-persistent.development.js +0.1% +0.1% 607.06 KB 607.52 KB 126.53 KB 126.66 KB NODE_DEV

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-test-utils.development.js 0.0% -0.0% 53.05 KB 53.05 KB 15.08 KB 15.08 KB NODE_DEV
react-dom.production.min.js 0.0% -0.0% 115.95 KB 115.96 KB 37.3 KB 37.3 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.59 KB 60.59 KB 15.99 KB 15.99 KB NODE_DEV
react-dom.profiling.min.js 0.0% -0.0% 119.57 KB 119.58 KB 38.46 KB 38.45 KB UMD_PROFILING
react-dom.development.js 0.0% +0.1% 953.59 KB 954.05 KB 213.85 KB 213.98 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 116.01 KB 116.02 KB 36.63 KB 36.64 KB NODE_PROD
react-dom.profiling.min.js 0.0% 0.0% 119.77 KB 119.78 KB 37.75 KB 37.76 KB NODE_PROFILING
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.2 KB 1.2 KB 703 B 702 B UMD_PROD
react-dom-server.browser.development.js 0.0% 0.0% 138.88 KB 138.88 KB 36.78 KB 36.78 KB UMD_DEV
react-dom-test-utils.development.js 0.0% -0.0% 54.78 KB 54.78 KB 15.4 KB 15.4 KB UMD_DEV
react-dom-server.browser.development.js 0.0% -0.0% 134.81 KB 134.81 KB 35.76 KB 35.75 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.89 KB 60.89 KB 16.07 KB 16.06 KB UMD_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.2 KB 1.2 KB 690 B 689 B NODE_PROD
react-dom-server.node.development.js 0.0% -0.0% 135.92 KB 135.92 KB 35.99 KB 35.99 KB NODE_DEV
react-dom.development.js 0.0% +0.1% 959.52 KB 959.98 KB 215.53 KB 215.66 KB UMD_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 20.34 KB 20.34 KB 7.55 KB 7.55 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.production.min.js 0.0% 0.0% 104.77 KB 104.78 KB 31.83 KB 31.84 KB UMD_PROD
react-art.development.js +0.1% +0.1% 678.18 KB 678.64 KB 145.96 KB 146.09 KB UMD_DEV
react-art.development.js +0.1% +0.1% 608.86 KB 609.32 KB 128.54 KB 128.67 KB NODE_DEV
react-art.production.min.js 0.0% 0.0% 69.77 KB 69.78 KB 21 KB 21.01 KB NODE_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.development.js 0.0% -0.0% 32.42 KB 32.42 KB 8.52 KB 8.52 KB NODE_DEV
react-test-renderer.development.js +0.1% +0.1% 622.95 KB 623.41 KB 131.49 KB 131.63 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 🔺+0.2% 71.91 KB 71.92 KB 21.92 KB 21.96 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 618.22 KB 618.68 KB 130.31 KB 130.45 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 0.0% 71.61 KB 71.62 KB 21.54 KB 21.55 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: -0.0%

Size changes (stable)

Generated by 🚫 dangerJS against 5f7361f

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 29, 2020

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 5f7361f:

Sandbox Source
silly-pike-jffqg Configuration

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.
@sizebot
Copy link

sizebot commented Jan 29, 2020

Details of bundled changes.

Comparing: 241c446...5f7361f

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 622.98 KB 623.44 KB 131.5 KB 131.64 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 🔺+0.2% 71.93 KB 71.94 KB 21.94 KB 21.98 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 37.89 KB 37.89 KB 9.84 KB 9.84 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.66 KB 11.66 KB 3.6 KB 3.6 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 32.43 KB 32.43 KB 8.53 KB 8.53 KB NODE_DEV
react-test-renderer.development.js +0.1% +0.1% 618.24 KB 618.7 KB 130.32 KB 130.46 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 0.0% 71.63 KB 71.64 KB 21.56 KB 21.56 KB NODE_PROD
ReactTestRenderer-dev.js +0.1% +0.1% 634.07 KB 634.53 KB 131.14 KB 131.29 KB FB_WWW_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.1% 751.01 KB 751.47 KB 157.94 KB 158.08 KB RN_OSS_DEV
ReactFabric-dev.js +0.1% +0.1% 741.63 KB 742.09 KB 155.73 KB 155.87 KB RN_OSS_DEV
ReactNativeRenderer-dev.js +0.1% +0.1% 751.19 KB 751.65 KB 158.03 KB 158.17 KB RN_FB_DEV
ReactFabric-prod.js 0.0% 0.0% 266.81 KB 266.84 KB 45.77 KB 45.78 KB RN_OSS_PROD
ReactNativeRenderer-prod.js 0.0% 0.0% 274.78 KB 274.8 KB 47.1 KB 47.11 KB RN_FB_PROD
ReactFabric-profiling.js 0.0% 0.0% 277.97 KB 278 KB 47.9 KB 47.91 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js 0.0% 0.0% 285.98 KB 286 KB 49.25 KB 49.25 KB RN_FB_PROFILING
ReactNativeRenderer-prod.js 0.0% 0.0% 274.39 KB 274.41 KB 47.03 KB 47.04 KB RN_OSS_PROD
ReactFabric-dev.js +0.1% +0.1% 741.81 KB 742.27 KB 155.81 KB 155.95 KB RN_FB_DEV
ReactNativeRenderer-profiling.js 0.0% 0.0% 285.59 KB 285.62 KB 49.18 KB 49.19 KB RN_OSS_PROFILING
ReactFabric-prod.js 0.0% 0.0% 267.16 KB 267.19 KB 45.84 KB 45.85 KB RN_FB_PROD
ReactFabric-profiling.js 0.0% 0.0% 278.31 KB 278.34 KB 47.97 KB 47.97 KB RN_FB_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js +0.1% +0.1% 622.39 KB 622.85 KB 128.58 KB 128.73 KB FB_WWW_DEV
ReactART-prod.js 0.0% 0.0% 236.29 KB 236.32 KB 39.8 KB 39.81 KB FB_WWW_PROD
react-art.development.js +0.1% +0.1% 678.2 KB 678.66 KB 145.97 KB 146.1 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 107.32 KB 107.33 KB 32.52 KB 32.52 KB UMD_PROD
react-art.development.js +0.1% +0.1% 608.88 KB 609.34 KB 128.54 KB 128.67 KB NODE_DEV
react-art.production.min.js 0.0% 0.0% 72.29 KB 72.29 KB 21.68 KB 21.69 KB NODE_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js 0.0% 0.0% 123.81 KB 123.82 KB 38.8 KB 38.8 KB NODE_PROFILING
react-dom-server.browser.development.js 0.0% 0.0% 138.9 KB 138.9 KB 36.78 KB 36.78 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20.47 KB 20.47 KB 7.5 KB 7.5 KB UMD_PROD
react-dom-test-utils.development.js 0.0% -0.0% 54.79 KB 54.79 KB 15.4 KB 15.4 KB UMD_DEV
ReactDOMServer-prod.js 0.0% 0.0% 49 KB 49 KB 11.19 KB 11.19 KB FB_WWW_PROD
react-dom-test-utils.production.min.js 0.0% -0.0% 11.18 KB 11.18 KB 4.15 KB 4.15 KB UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.21 KB 1.21 KB 711 B 710 B UMD_PROD
react-dom-test-utils.development.js 0.0% -0.0% 53.06 KB 53.06 KB 15.08 KB 15.08 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.95 KB 10.95 KB 4.09 KB 4.09 KB NODE_PROD
react-dom.development.js 0.0% +0.1% 959.54 KB 960.01 KB 215.55 KB 215.68 KB UMD_DEV
react-dom.production.min.js 0.0% 0.0% 119.85 KB 119.86 KB 38.42 KB 38.42 KB UMD_PROD
react-dom.profiling.min.js 0.0% 0.0% 123.58 KB 123.59 KB 39.61 KB 39.61 KB UMD_PROFILING
react-dom.development.js 0.0% +0.1% 953.61 KB 954.07 KB 213.87 KB 214 KB NODE_DEV
react-dom-server.node.development.js 0.0% -0.0% 135.95 KB 135.95 KB 35.99 KB 35.99 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 119.93 KB 119.94 KB 37.68 KB 37.68 KB NODE_PROD
react-dom-server.browser.development.js 0.0% -0.0% 134.84 KB 134.84 KB 35.76 KB 35.76 KB NODE_DEV
ReactDOM-dev.js 0.0% +0.1% 981.24 KB 981.7 KB 216.73 KB 216.87 KB FB_WWW_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20.39 KB 20.39 KB 7.47 KB 7.47 KB NODE_PROD
ReactDOM-prod.js 0.0% 0.0% 393.42 KB 393.45 KB 71.88 KB 71.89 KB FB_WWW_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.91 KB 60.91 KB 16.07 KB 16.07 KB UMD_DEV
ReactDOM-profiling.js 0.0% 0.0% 404.75 KB 404.77 KB 74.05 KB 74.05 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.61 KB 60.61 KB 16 KB 15.99 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 4.42 KB 4.42 KB 1.65 KB 1.65 KB NODE_DEV
ReactDOMServer-dev.js 0.0% 0.0% 140.24 KB 140.24 KB 35.49 KB 35.49 KB FB_WWW_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.21 KB 1.21 KB 698 B 697 B NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.development.js +0.1% +0.1% 607.08 KB 607.54 KB 126.53 KB 126.66 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% -0.0% 19.53 KB 19.53 KB 6.4 KB 6.39 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% 0.0% 72.53 KB 72.54 KB 21.32 KB 21.33 KB NODE_PROD
react-reconciler.development.js +0.1% +0.1% 609.88 KB 610.34 KB 127.71 KB 127.84 KB NODE_DEV
react-reconciler.production.min.js 0.0% 0.0% 75.29 KB 75.3 KB 21.96 KB 21.96 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: -0.0%

Size changes (experimental)

Generated by 🚫 dangerJS against 5f7361f

@acdlite acdlite changed the title Failing test: Expiring a partially completed tree Bugfix: Expiring a partially completed tree Jan 29, 2020
@acdlite acdlite merged commit 01974a8 into facebook:master Jan 29, 2020
acdlite added a commit to acdlite/react that referenced this pull request Jan 30, 2020
This reverts commit 01974a8.

* 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
acdlite added a commit that referenced this pull request Jan 30, 2020
This reverts commit 01974a8.

* 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
acdlite added a commit to acdlite/react that referenced this pull request Jan 31, 2020
* 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
acdlite added a commit to acdlite/react that referenced this pull request Feb 4, 2020
* 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
acdlite added a commit to acdlite/react that referenced this pull request Feb 5, 2020
* 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
acdlite added a commit to acdlite/react that referenced this pull request Feb 26, 2020
* 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
acdlite added a commit to acdlite/react that referenced this pull request Mar 3, 2020
* 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
acdlite added a commit to acdlite/react that referenced this pull request Mar 3, 2020
* 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
acdlite added a commit that referenced this pull request Mar 3, 2020
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants