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

Use a single write transaction for DiscardLocal client resets on FLX realms #7110

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Nov 2, 2023

Updating the subscription store in a separate write transaction from the recovery means that we temporarily commit an invalid state. If the application crashes between committing the client reset diff and updating the subscription store, the next launch of the application would try to use the now-invalid pending subscriptions that should have been discarded.

This is the fairly uninteresting case; client reset with recovery can do much more invalid things than this if the process crashes. That'll be a significantly more involved thing to fix so I'm splitting it off to a subsequent PR.

@tgoyne tgoyne self-assigned this Nov 2, 2023
Copy link

coveralls-official bot commented Nov 2, 2023

Pull Request Test Coverage Report for Build github_pull_request_282770

  • 141 of 146 (96.58%) changed or added relevant lines in 7 files are covered.
  • 44 unchanged lines in 12 files lost coverage.
  • Overall coverage increased ( 0.02%) to 91.677%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/realm/sync/subscriptions.cpp 33 38 86.84%
Files with Coverage Reduction New Missed Lines %
src/realm/sync/subscriptions.cpp 1 94.41%
src/realm/array_string.cpp 2 88.0%
src/realm/query_expression.hpp 2 93.51%
src/realm/sync/noinst/client_impl_base.cpp 2 85.32%
src/realm/sync/noinst/client_reset.cpp 2 93.69%
src/realm/util/file.cpp 2 81.73%
src/realm/util/serializer.cpp 3 90.03%
test/fuzz_group.cpp 3 54.09%
src/realm/sync/transform.cpp 4 63.08%
src/realm/alloc_slab.cpp 5 92.89%
Totals Coverage Status
Change from base Build 1813: 0.02%
Covered Lines: 230762
Relevant Lines: 251711

💛 - Coveralls

@tgoyne tgoyne requested a review from ironage November 3, 2023 00:59
Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Good readability improvement to perform_client_reset_diff as well.

…realms

Updating the subscription store in a separate write transaction from the
recovery means that we temporarily commit an invalid state. If the application
crashes between committing the client reset diff and updating the subscription
store, the next launch of the application would try to use the now-invalid
pending subscriptions that should have been discarded.
@tgoyne tgoyne closed this Nov 3, 2023
@tgoyne tgoyne reopened this Nov 3, 2023
@tgoyne tgoyne merged commit 3571738 into master Nov 4, 2023
35 checks passed
@tgoyne tgoyne deleted the tg/client-reset-flx branch November 4, 2023 00:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants