RCORE-2160 Make upload completion reporting multiprocess-compatible #7796
Conversation
| REALM_ASSERT(self->m_actualized); | ||
| if (!status.is_ok()) { |
There was a problem hiding this comment.
If post() itself failed we previously never called the completion callback, while now we report the error to the callback. The event loop being able to fail is sort of weird and I'm not sure it can actually happen?
There was a problem hiding this comment.
I'd expect the only error we could get here to be OperationAborted if the event loop were shut down before the sync client.
| @@ -1564,6 +1497,23 @@ void SessionWrapper::force_close() | |||
| m_sess = nullptr; | |||
| // Everything is being torn down, no need to report connection state anymore | |||
| m_connection_state_change_listener = {}; | |||
|
|
|||
| // All outstanding wait operations must be canceled | |||
There was a problem hiding this comment.
Moving this from finalize() to force_close() means that in tests we send the notifications when the client is shutdown rather than when the session is abandoned, matching the old behavior of blocking wait for completion or client stop. I think this is clearly correct for tests and should have no effect outside of test code.
| @@ -730,6 +730,7 @@ TEST_CASE("flx: client reset", "[sync][flx][client reset][baas]") { | |||
| REQUIRE(mode == ClientResyncMode::Recover); | |||
| auto subs = local_realm->get_latest_subscription_set(); | |||
| subs.get_state_change_notification(sync::SubscriptionSet::State::Complete).get(); | |||
| subs.refresh(); | |||
There was a problem hiding this comment.
This test was relying on wait_for_upload_completion() waiting for subscription changes to be uploaded (which it would only sometimes do and wasn't actually guaranteed), which happened to result in the subscription state being Complete before the call to get_state_change_notification() so this worked without the refresh.
There was a problem hiding this comment.
has this been flaky lately or something? i think waiting_for_upload_completion() used to guarantee this, but maybe that's changed from under me.
There was a problem hiding this comment.
check_for_upload_completion() has had specific logic to report completion even if there's unuploaded changesets as long as it had scanned all of the changesets (i.e. as long as all of the remaining ones are empty or from the server) since 2018, and I'm pretty sure there was equivalent behavior achieved differently before that.
The change in functionality that broke this test is that we don't scan the changesets to see if any needed to be uploaded until after the first DOWNLOAD is received, so previously empty changesets made wait_for_uploads() wait for the first DOWNLOAD message and now it doesn't. It's probably possible to preserve that behavior, but it seems really weird and inconsistent (particularly because the presence of empty changesets may not be directly related to anything the developer did).
Either way, the test was incorrect; it should either be waiting on the state change notification and then calling refresh or simply asserting the state without waiting. Waiting then asserting without the refresh in between doesn't really make any sense.
| @@ -737,7 +737,7 @@ struct BaasFLXClientReset : public TestClientReset { | |||
| if (m_on_post_local) { | |||
| m_on_post_local(realm); | |||
| } | |||
| wait_for_upload(*realm); | |||
| wait_for_download(*realm); | |||
There was a problem hiding this comment.
This was relying on wait_for_upload() waiting for subscription changes after resume(). The thing we're actually waiting for here is for is a server roundtrip so that we receive the client reset error, which wait_for_download() does guarantee.
tgoyne
force-pushed
the
tg/upload-completion
branch
from
961d9d7
to
61c2bed
Compare
tgoyne
force-pushed
the
tg/download-progress
branch
from
5680af0
to
caff9c2
Compare
tgoyne
force-pushed
the
tg/upload-completion
branch
2 times, most recently
from
6446366
to
8213071
Compare
tgoyne
force-pushed
the
tg/download-progress
branch
from
caff9c2
to
6501c2c
Compare
tgoyne
force-pushed
the
tg/upload-completion
branch
from
8213071
to
f29c23c
Compare
Pull Request Test Coverage Report for Build thomas.goyne_416Details
💛 - Coveralls |
tgoyne
force-pushed
the
tg/download-progress
branch
from
6501c2c
to
5931242
Compare
tgoyne
force-pushed
the
tg/upload-completion
branch
from
f29c23c
to
3657ee6
Compare
Pull Request Test Coverage Report for Build thomas.goyne_419Details
💛 - Coveralls |
tgoyne
force-pushed
the
tg/upload-completion
branch
from
3657ee6
to
b9cd461
Compare
Pull Request Test Coverage Report for Build thomas.goyne_420Details
💛 - Coveralls |
| if (uploaded_version == current_client_version) | ||
| return; | ||
|
|
||
| BinaryColumn changesets(db.get_alloc()); |
There was a problem hiding this comment.
you can use m_array->changesets and m_arrays->origin_file_idents
There was a problem hiding this comment.
This is a static function.
| // empty changesets and did not need to be uploaded. If this is less than | ||
| // uploaded_version, we have changesets which have been uploaded but the | ||
| // server has not yet told us we can delete and we may need to use for merging. | ||
| auto base_version = current_client_version - changesets.size(); |
There was a problem hiding this comment.
I think you should use m_sync_history_base_version here instead
| } | ||
|
|
||
| auto count = size_t(current_client_version - uploaded_version); | ||
| for (size_t i = changesets.size() - count; i < changesets.size(); ++i) { |
There was a problem hiding this comment.
you can take a look at the loop in trim_sync_history() since you're doing something similar
There was a problem hiding this comment.
I don't know what this comment means. I have indeed looked at that loop?
There was a problem hiding this comment.
I missed that the function is static. I meant that you could make the loop pretty much the same.
|
|
||
| void on_upload_completion(); | ||
| version_type m_upload_completion_requested_version = -1; | ||
|
|
||
| void on_download_completion(); |
There was a problem hiding this comment.
It'd be nice to align all completion handlers at some point given the current refactoring.
There was a problem hiding this comment.
The end state of all this does need to be that download completion is also determinable by inspecting the Realm file, but it's significantly more complicated to get there for downloads (as the server is the source of truth for download completion rather than the client). I'm trying to split off each of the separate pieces to avoid having another monster PR that changes everything.
tgoyne
force-pushed
the
tg/upload-completion
branch
from
b9cd461
to
c1921b1
Compare
…ppers around async completion
Rather than tracking a bunch of derived state in-memory, check for upload completion by checking if there are any unuploaded changesets. This is both multiprocess-compatible and is more precise than the old checks, which had some false-negatives.
tgoyne
force-pushed
the
tg/upload-completion
branch
from
c1921b1
to
c6b7d3d
Compare
Pull Request Test Coverage Report for Build thomas.goyne_425Details
💛 - Coveralls |
Rather than tracking a bunch of derived state in-memory, check for upload completion by checking if there are any unuploaded changesets. This is both multiprocess-compatible and is more precise than the old checks, which had some false-negatives and minor inconsistencies. Previously creating local commits which produced empty changesets and then calling wait_for_upload_completion() would complete immediately, but pausing and then resuming the session would make it wait until the new session performed the upload scan, which didn't happen until after download completion.
The synchronous completion waits (which are hopefully only used in tests) are now just thin wrappers around the async waits. This exposed a small inconsistency around when completion happened when the sync client is stopped, which is something we don't expose publicly so changing it should be fine.