-
Notifications
You must be signed in to change notification settings - Fork 163
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
Conversation
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1272Details
💛 - Coveralls |
{ | ||
// Verify that the file was fully closed | ||
auto empty = [](auto&) {}; | ||
REQUIRE(DB::call_with_lock(interrupted_realm_config.path, empty)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…ze-sync-version-assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 callenlist_to_send()
so the assert can be removed.NOTES:
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.make_client_reset_handler()
function fromflx_sync.cpp
tosync_test_utils.hpp
was copied from the Role Changes without a client reset feature branch.Fixes #7860
☑️ ToDos
[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml
, if public C++ API changed