[go: up one dir, main page]

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

RCORE-2190 Sync client can crash if a session is resumed before UNBIND message finishes sending #7874

Merged
merged 11 commits into from
Jul 16, 2024

Conversation

michael-wb
Copy link
Contributor
@michael-wb michael-wb commented Jul 9, 2024

What, How & Why?

An occasional assert was failing during the FLX migration tests where handle_reconnect() was being called while the current session was restarting to perform a PBS->FLX migration. These changes add a test to reproduce this issue and updates the logic around when to call enlist_to_send() so the assert can be removed.

NOTES:

  • the "sync: pending client resets are cleared when downloads are complete" test was updated, since it really wasn't doing anything. The client reset was being triggered, but the session needed to be restarted to actually perform the client reset.
  • the changes to move the make_client_reset_handler() function from flx_sync.cpp to sync_test_utils.hpp was copied from the Role Changes without a client reset feature branch.

Fixes #7860

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • [ ] C-API, if public C++ API changed
  • [ ] bindgen/spec.yml, if public C++ API changed

@michael-wb michael-wb self-assigned this Jul 9, 2024
@cla-bot cla-bot bot added the cla: yes label Jul 9, 2024
Copy link
coveralls-official bot commented Jul 10, 2024

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1272

Details

  • 113 of 119 (94.96%) changed or added relevant lines in 4 files are covered.
  • 42 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+0.002%) to 90.995%

Changes Missing Coverage Covered Lines Changed/Added Lines %
test/object-store/sync/client_reset.cpp 90 96 93.75%
Files with Coverage Reduction New Missed Lines %
src/realm/index_string.cpp 1 85.17%
src/realm/index_string.hpp 1 93.48%
src/realm/uuid.cpp 1 98.48%
test/test_dictionary.cpp 1 99.83%
src/realm/query_expression.hpp 2 93.81%
src/realm/table_view.cpp 2 92.99%
test/object-store/sync/client_reset.cpp 2 99.03%
test/object-store/sync/flx_sync.cpp 2 98.37%
src/realm/sync/transform.cpp 3 60.99%
test/fuzz_tester.hpp 4 57.32%
Totals Coverage Status
Change from base Build 2492: 0.002%
Covered Lines: 215490
Relevant Lines: 236814

💛 - Coveralls

{
// Verify that the file was fully closed
auto empty = [](auto&) {};
REQUIRE(DB::call_with_lock(interrupted_realm_config.path, empty));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change? How is this call different than what was there before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't really change anything
VS Code was reporting an error for this line: a lambda is not allowed in an unevaluated expressionC/C++(1777)

realm_config.sync_config->notify_after_client_reset = after_reset;
auto realm = Realm::get_shared_realm(realm_config);
wait_for_download(*realm, std::chrono::minutes(10));
REQUIRE(future.is_ready());
Copy link
Contributor

Choose a reason for hiding this comment

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

What are we checking here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its supposed to verify that the client reset is complete (e.g. notify_after_client_reset callback has already been called and the future is ready) by the time the wait_for_download() returns after the client reset is performed.

I am assuming that was the original intent for this test, but the original test wasn't actually performing any client resets.

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the original test was calling trigger_client_reset, did that not work for some reason? or was it just not checking that a client reset actually happened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trigger_client_reset() just invalidates the file ident and the client needs to reconnect before it will receive the client reset error.

Copy link
Contributor

Choose a reason for hiding this comment

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

so this way we're actually triggering a client reset because we close the realm each time by putting the realm in separate scopes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - since it's using the same config, it should be reusing the same realm file/file ident.
Would you prefer me to use pause()/resume() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also needed to close the realm so I could add a new client reset handler and future before opening it again.

CHANGELOG.md Outdated Show resolved Hide resolved
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.

LGTM

@michael-wb michael-wb merged commit e019c79 into master Jul 16, 2024
41 checks passed
@michael-wb michael-wb deleted the mwb/recognize-sync-version-assert branch July 16, 2024 21:59
@github-actions github-actions bot mentioned this pull request Jul 19, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync client can crash if a session is resumed before UNBIND message finishes sending
4 participants