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 incorrect rebootstrap #7238

Merged
merged 5 commits into from
Jan 11, 2024
Merged

Fix incorrect rebootstrap #7238

merged 5 commits into from
Jan 11, 2024

Conversation

danieltabacaru
Copy link
Collaborator

@danieltabacaru danieltabacaru commented Jan 8, 2024

What, How & Why?

Pausing and resuming the sync session at a very specific time may cause the next download message to be mistaken for a bootstrap message and crash the application.

Fixes #6611.

☑️ ToDos

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

Copy link

coveralls-official bot commented Jan 8, 2024

Pull Request Test Coverage Report for Build daniel.tabacaru_696

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 91.856%

Totals Coverage Status
Change from base Build 1954: 0.06%
Covered Lines: 233083
Relevant Lines: 253749

💛 - Coveralls

return SyncClientHookAction::NoAction;
};
auto realm = Realm::get_shared_realm(triggered_config);
interrupted.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to wait for the bootstrap to finish here for the test to really validate whether this fix has worked?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a matter of fact the test does finish, but it's not immediately obvious why. I think it helps the readability if we fulfil the promise after the message is integrated (I'll add some comments too).

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check here that the subscription set version is zero and the subscription set state is Complete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do

Copy link
Contributor

Choose a reason for hiding this comment

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

thinking about it more, do we even need the promise in that case. could the test outside the event hook just wait for the subscription set to be complete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no. the subscription set is complete as part of the first session, but the second session and its session wrapper used to read the state of the subscription store at the wrong time.

@danieltabacaru danieltabacaru merged commit a558fa4 into master Jan 11, 2024
34 checks passed
@danieltabacaru danieltabacaru deleted the dt/fix_wrong_rebootstrap branch January 11, 2024 21:19
@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.

Assertion failed: incoming_changesets.size() != 0
2 participants