RCORE-1977 Empty reciprocal changesets lead to crashes or data divergence #7955
Conversation
Pull Request Test Coverage Report for Build daniel.tabacaru_884Details
💛 - Coveralls |
| sync::parse_changeset(in, reciprocal_changeset); // Throws | ||
| } | ||
| // The only instruction in the reciprocal changeset was discarded during OT. | ||
| CHECK(reciprocal_changeset.empty()); |
There was a problem hiding this comment.
the comment above says there should be two reciprocal changesets?
| return ++timestamp; | ||
| }); | ||
|
|
||
| auto latest_local_version = [&] { |
There was a problem hiding this comment.
it looks like this gets used by all three of these tests, maybe move it into its own static function?
There was a problem hiding this comment.
it's not the exact setup for all three tests, but I can look into refactoring it (or perhaps split it into two part: schema creation and adding the initial data)
There was a problem hiding this comment.
Ah, got it. I will leave whether to refactor this to your judgement.
There was a problem hiding this comment.
decided it's not worth it. I'd've normally implemented this using sections. maybe we should switch to using catch as well.
| auto transact = db->start_read(); | ||
| history.integrate_server_changesets(progress, downloadable_bytes, server_changesets_encoded, version_info, | ||
| DownloadBatchState::SteadyState, *test_context.logger, transact); | ||
|
|
There was a problem hiding this comment.
should we check that the reciprocal changesets are what we expect here?
There was a problem hiding this comment.
we can do that
| history.integrate_server_changesets(progress, downloadable_bytes, server_changesets_encoded, version_info, | ||
| DownloadBatchState::SteadyState, *test_context.logger, transact); | ||
| } | ||
|
|
There was a problem hiding this comment.
same q here, do we need to check that the reciprocal changesets are what we expect?
src/realm/sync/transform.cpp
Outdated
| @@ -2591,7 +2591,8 @@ Changeset& Transformer::get_reciprocal_transform(TransformHistory& history, file | |||
| version_type version, const HistoryEntry& history_entry) | |||
| { | |||
| auto& changeset = m_reciprocal_transform_cache[version]; // Throws | |||
| if (changeset.empty()) { | |||
| // There can be empty changesets in the cache, so check the version too. | |||
| if (changeset.empty() && changeset.version == 0) { | |||
There was a problem hiding this comment.
remind me, how do you end up with a changeset in the cache where the version is zero?
There was a problem hiding this comment.
the instruction above it inserts an empty changeset (with version zero) if the key is not found. subtle, but one the culprits. We used emplace() before, but it was changed/refactored with a bunch of other changes some time ago and so the bug was introduced.
There was a problem hiding this comment.
Would you mind updating the comment to explain this? Thanks
There was a problem hiding this comment.
interesting. is there a reason why we can't use cache.insert right above rather than the subscript operator so we actually get the inserted flag rather than checking if the version is zero?
src/realm/sync/transform.cpp
Outdated
| @@ -2591,7 +2591,8 @@ Changeset& Transformer::get_reciprocal_transform(TransformHistory& history, file | |||
| version_type version, const HistoryEntry& history_entry) | |||
| { | |||
| auto& changeset = m_reciprocal_transform_cache[version]; // Throws | |||
| if (changeset.empty()) { | |||
| // There can be empty changesets in the cache, so check the version too. | |||
| if (changeset.empty() && changeset.version == 0) { | |||
There was a problem hiding this comment.
Would you mind updating the comment to explain this? Thanks
What, How & Why?
This PR fixes two bugs related to storing and retrieving reciprocal changesets:
Fixes #7893.
☑️ ToDos
[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml, if public C++ API changed