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

Fix useMutableSource tearing bug #18912

Merged
merged 2 commits into from May 13, 2020
Merged

Fix useMutableSource tearing bug #18912

merged 2 commits into from May 13, 2020

Conversation

@acdlite
Copy link
Member

@acdlite acdlite commented May 13, 2020

If a source is mutated after initial read but before subscription is set up, it should still entangle all pending mutations even if snapshot of new subscription happens to match.

Test case illustrates how not doing this can lead to tearing.

The fix is to move the entanglement call outside of the block that checks if the snapshot has changed.

acdlite added 2 commits May 13, 2020
If a source is mutated after initial read but before subscription is set
up, it should still entangle all pending mutations even if snapshot of
new subscription happens to match.

Test case illustrates how not doing this can lead to tearing.
Fix is to move the entanglement call outside of the block that checks
if the snapshot has changed.
@codesandbox
Copy link

@codesandbox codesandbox bot commented May 13, 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 43ce30b:

Sandbox Source
gracious-ritchie-6hofe Configuration
@codesandbox
Copy link

@codesandbox codesandbox bot commented May 13, 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 c3c4c3f:

Sandbox Source
sweet-dubinsky-bqbis Configuration
@sizebot
Copy link

@sizebot sizebot commented May 13, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against c3c4c3f

@sizebot
Copy link

@sizebot sizebot commented May 13, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against c3c4c3f

Copy link
Contributor

@bvaughn bvaughn left a comment

This looks like an improvement!

Correct me if I'm wrong though, but this tearing case would still be possible for any source where version is not an auto-incremented value (e.g. window.location, maybe even Redux stores).

// There is no mechanism currently to associate these updates though,
// so for now we fall back to synchronously flushing all pending updates.
// TODO: Improve this later.
markRootExpiredAtTime(root, getLastPendingExpirationTime(root));

This comment has been minimized.

@bvaughn

bvaughn May 13, 2020
Contributor

Ah, that's unfortunate. Even more deopts in old mode now.

@acdlite
Copy link
Member Author

@acdlite acdlite commented May 13, 2020

Correct me if I'm wrong though, but this tearing case would still be possible for any source where version is not an auto-incremented value (e.g. window.location, maybe even Redux stores).

Yeah sounds right to me. Something to think about.

@acdlite acdlite merged commit fdb6416 into facebook:master May 13, 2020
31 checks passed
31 checks passed
ci/circleci: RELEASE_CHANNEL_stable_yarn_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_persistent Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod_www_variant Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/circleci: build_devtools_and_process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_experimental Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot_experimental Your tests passed on CircleCI!
Details
ci/circleci: sizebot_stable Your tests passed on CircleCI!
Details
ci/circleci: yarn_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_flow Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod_www_variant Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.