Distinguish between an empty and a null (not existing) reciprocal changeset

[danieltabacaru]

What, How & Why?

Once a reciprocal changeset has all its instructions discarded during OT, it is stored as a null changeset in the realm. This makes it hard to differentiate it from a non-existing reciprocal changeset and was causing histories to diverge among clients. A fix was put in place so that an empty reciprocal changeset is indeed saved as an empty changeset in the realm.

Fixes #6191.

☑️ ToDos

• 📝 Changelog update
• 🚦 Tests (or not relevant)
• C-API, if public C++ API changed.

[michael-wb]

Looks good - I'm surprised this hasn't manifested in other ways than just the list out of order, although that might be the easiest one to detect.

[michael-wb]

Update this to CHECK() or REQUIRE() so it integrates better into the test.

[jbreams]

This is an excellent find, my only question is whether we're fixing this at the right place. Is it ever valid to call TransformHistory::set_reciprocal_transform() with a null BinaryData? Somehow changing the encoding of changesets so they contain intern strings whose values are never referenced doesn't feel right to me. What if instead of changing the encoding of changesets we made it so ClientHistory::set_reciprocal_transform() treated a null BinaryData as a non-null empty BinaryData? Maybe something like this?

void ClientHistory::set_reciprocal_transform(version_type version, BinaryData data)
{
    REALM_ASSERT(version > m_sync_history_base_version);
    std::size_t index = size_t(version - m_sync_history_base_version) - 1;
    REALM_ASSERT(index < sync_history_size());

    if (data.size() == 0) {
        m_arrays->reciprocal_transforms.set(index, BinaryData("", 0));
        return;
    }

    auto compressed = util::compression::allocate_and_compress_nonportable(data);
    m_arrays->reciprocal_transforms.set(index, BinaryData{compressed.data(), compressed.size()}); // Throws
}

Also, do we want to include the reproduction from the original issue here that actually instantiates some arrays and does moves on them as a slightly higher level integration test?

[tgoyne]

It looks like this bug was introduced in ae2e6d3. I removed the check which ensured that empty data was stored as empty instead of null because the compressed data would always have a header, and then changed it so that zero byte input compressed to zero byte output and made the check necessary again. add_sync_history_entry() may also need the check restored.

[danieltabacaru]

It looks like this bug was introduced in ae2e6d3. I removed the check which ensured that empty data was stored as empty instead of null because the compressed data would always have a header, and then changed it so that zero byte input compressed to zero byte output and made the check necessary again. add_sync_history_entry() may also need the check restored.

Thanks a lot for the insight. I restored the checks as they were before ae2e6d3.

[danieltabacaru]

This is an excellent find, my only question is whether we're fixing this at the right place. Is it ever valid to call TransformHistory::set_reciprocal_transform() with a null BinaryData? Somehow changing the encoding of changesets so they contain intern strings whose values are never referenced doesn't feel right to me. What if instead of changing the encoding of changesets we made it so ClientHistory::set_reciprocal_transform() treated a null BinaryData as a non-null empty BinaryData? Maybe something like this?

void ClientHistory::set_reciprocal_transform(version_type version, BinaryData data)
{
    REALM_ASSERT(version > m_sync_history_base_version);
    std::size_t index = size_t(version - m_sync_history_base_version) - 1;
    REALM_ASSERT(index < sync_history_size());

    if (data.size() == 0) {
        m_arrays->reciprocal_transforms.set(index, BinaryData("", 0));
        return;
    }

    auto compressed = util::compression::allocate_and_compress_nonportable(data);
    m_arrays->reciprocal_transforms.set(index, BinaryData{compressed.data(), compressed.size()}); // Throws
}

Also, do we want to include the reproduction from the original issue here that actually instantiates some arrays and does moves on them as a slightly higher level integration test?

I wanted to add that specific test but I need control on the changesets' timestamp to hit the critical code path which is not straightforward.

[jbreams]

Can you add something here like - this may have resulted in arrays being in different orders on different devices - so that users reading this will know what symptom to look for?