Fix streaming download notifiers #5008

tgoyne · 2021-10-29T20:22:47Z

#4989 shifted where downloaded_bytes + downloadable_bytes was done, which resulted in streaming download notifiers reporting incorrect values. This moves it back and adds a different fix for what appears to be the bug it was trying to fix.

jbreams · 2021-10-29T20:34:42Z

src/realm/object-store/sync/sync_session.cpp

+            return [] {};
+
+        // The initial download size we get from the server is an estimate
+        // and it may decrease once compaction is performed, so we need to


This is not true. Once compaction is performed, the downloadable bytes can be greater than the actual number of bytes available, and so we'll never reach it. The only guarantee the server makes about downloadable_bytes is that it will be zero when there are no more download messages to send. That's what the original PR was trying to address.

I don't understand how that's different? Receiving downloadable_bytes=0 from the server makes the value we get here go down.

I suppose we could special-case getting downloadable_bytes=0 rather than just handling any values which has shrunk by more than downloaded_bytes increased by, but that just seems worse and then we'd need error-handling for the case where it decreased by too much but not to zero.

In practice, you're right, but this is not the guarantee the server actually gives us. The only guarantee we get is that downloadable_bytes will be zero when there are no more download messages to send.

If the contract with the server is that it sends a random value in downloadable_bytes if there is more to download and zero if there is not, then the server has made it impossible to implement non-streaming progress notifiers (or useful progress notifiers at all), and the protocol is completely insane.

…hanges. (#4989)" This reverts commit e5a115f.

jbreams · 2021-10-30T02:04:32Z

src/realm/sync/client.cpp

@@ -1226,17 +1226,27 @@ void SessionWrapper::report_progress()
    ClientHistory::get_upload_download_bytes(m_db.get(), downloaded_bytes, downloadable_bytes, uploaded_bytes,
                                             uploadable_bytes, snapshot_version);

+    // In protocol versions 25 and earlier, downloadable_bytes was the total


This comment is nonsensical wherever it is and I should have asked to remove it before. The current protocol version is 3. There is no protocol version 25 or protocol version 26. They are dead and buried deep in the ground.

We do this addition here to hide the real meaning of these values from object-store.

jbreams · 2021-10-30T02:07:17Z

src/realm/sync/client.cpp

+    // meaning of downloadable_bytes for the progress handler, but that would be
+    // a breaking change. Note that protocol version 25 (and earlier) is no
+    // longer supported by clients.
+    std::uint_fast64_t total_bytes = downloaded_bytes + downloadable_bytes;


I dislike that we're layering this information so that something that's higher in the call stack of the same statically linked library is unaware of the meaning of the information being passed to it. I think this adds complexity for no reason and makes this confusing and error prone.

I don't particularly think that this is the right place to do this, but only because I think it should be done even earlier. The exact details of how progress is represented in the protocol are not relevant to anything which isn't interacting directly with the protocol, which includes most of the sync client.

jbreams · 2021-10-30T02:07:56Z

src/realm/sync/client.cpp

-    std::uint_fast64_t download_version = (m_reliable_download_progress ? 1 : 0);
-    m_progress_handler(downloaded_bytes, downloadable_bytes, uploaded_bytes, uploadable_bytes, download_version,
+    // FIXME: Why is this boolean status communicated to the application as
+    // a 64-bit integer? Also, the name `progress_version` is confusing.


We don't need to change everything in this PR, but we def don't need to make existing passive aggressive comments into outwardly aggressive comments. Either fix this or just remove the comment.

This is pre-existing code which I didn't touch.

jbreams · 2021-10-30T02:22:18Z

src/realm/object-store/sync/sync_session.cpp

+            return [] {};
+
+        // The initial download size we get from the server is an estimate
+        // and it may decrease once compaction is performed, so we need to


In practice, you're right, but this is not the guarantee the server actually gives us. The only guarantee we get is that downloadable_bytes will be zero when there are no more download messages to send.

jbreams · 2021-10-30T02:34:49Z

test/object-store/sync/session/progress_notifications.cpp

+            CHECK(!callback_was_called_2);
+        }
+
+        SECTION("download notifiers handle transferrable decreasing") {


I'm sure that there's a test case that would have caught this issue and that it somehow slipped through CR, but could you point out to me where it is in this file?

The tests did catch that there was a functional change, and then the tests were modified to pass.

…ownloaded

tgoyne · 2021-11-01T16:47:25Z

So, now that I'm actually awake and less grumpy:

The general problem with #4989 is the changes to the tests. Shifting where the addition is done required updating all of the existing download notifier tests, and the nature of those tests makes it very difficult to verify that the updates were correct (and they turned out not to be). In addition, rather than adding a new test to cover the scenario which previously wasn't tested, an existing test was modified. Because of these, I felt that the most straightforward way to fix the problem was to revert those changes, do a different change that didn't require updating existing tests, and add a new test for the previously untested scenario.

The change in #4989 also introduced a confusing inconsistency: in the progress notifications produced by SessionWrapper, downloadable_bytes included only the remaining bytes to download, but uploadable_bytes was the total bytes which would be uploaded. These two things are equivalent and can be trivially converted to each other, but the two fields should obviously use the same representation. Aligning these two really ought to be done before writing them to the file rather than when they're read, but that's maybe a bigger change.

If the server today will never decrease downloadable_bytes by more than downloaded_bytes increased except for in the specific case where downloadable_bytes goes to zero then there is no need for us to handle the more general case, but I see no downside to us handling the more general case and it leaves us prepared for if the server is one day able to send more precise information.

jbreams

Now that it is not Friday night and I am also less grumpy...

There are still things about this that I want to change, but they don't need to be done here, and I don't want to hold up fixing regressions.

Thanks for taking care of this.

tgoyne self-assigned this Oct 29, 2021

cla-bot bot added the cla: yes label Oct 29, 2021

tgoyne mentioned this pull request Oct 29, 2021

Upgrade to core 11.6.0 realm/realm-swift#7488

Merged

jbreams reviewed Oct 29, 2021

View reviewed changes

tgoyne force-pushed the tg/streaming-notifier branch from e813638 to 92b0ef9 Compare October 29, 2021 22:15

tgoyne added 2 commits October 29, 2021 16:27

Revert "Update captured_transferrable when downloadable bytes value c…

36e1884

…hanges. (#4989)" This reverts commit e5a115f.

Handle downloadable_bytes decreasing for non-streaming notifiers

0ff0059

tgoyne force-pushed the tg/streaming-notifier branch from 92b0ef9 to 0ff0059 Compare October 29, 2021 23:27

jbreams requested changes Oct 30, 2021

View reviewed changes

Add a more accurate description of why we're adding downloadable to d…

094db34

…ownloaded

jbreams approved these changes Nov 1, 2021

View reviewed changes

tgoyne merged commit 74f26c3 into master Nov 1, 2021

tgoyne deleted the tg/streaming-notifier branch November 1, 2021 19:28

ironage mentioned this pull request Nov 2, 2021

Upgrade to Realm Core 11.6.0. Was: Adopting to changes in Realm Core (past 11.4.1) realm/realm-js#4023

Merged

8 tasks

github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix streaming download notifiers #5008

Fix streaming download notifiers #5008

tgoyne commented Oct 29, 2021

jbreams Oct 29, 2021

tgoyne Oct 29, 2021

tgoyne Oct 29, 2021

jbreams Oct 30, 2021

tgoyne Oct 30, 2021

jbreams Oct 30, 2021

jbreams Oct 30, 2021

tgoyne Oct 30, 2021

jbreams Oct 30, 2021

tgoyne Oct 30, 2021

jbreams Oct 30, 2021

jbreams Oct 30, 2021

tgoyne Oct 30, 2021

tgoyne commented Nov 1, 2021

jbreams left a comment

Fix streaming download notifiers #5008

Fix streaming download notifiers #5008

Conversation

tgoyne commented Oct 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgoyne commented Nov 1, 2021

jbreams left a comment

Choose a reason for hiding this comment