realm-core icon indicating copy to clipboard operation
realm-core copied to clipboard

SyncSession::wait_for_upload_completion does not return error if session is invalid

Open cmelchior opened this issue 2 years ago • 8 comments

While trying to write some unit tests for Kotlin around waiting for upload and download I discovered something that looks like a bug.

  1. Open a synchronized Realm
  2. Restart Sync on the server using the Admin API, this correctly triggers a session error handler.
  3. Attempt to call SyncSession: wait_for_upload_completion and SyncSession::wait_for_download_completion

Expected result: I would expect the function to return some kind of error_code in the callback we provide.

Actual result: It returns succesfully.

cmelchior avatar Mar 30 '22 09:03 cmelchior

Does the callback return successfully or does just calling wait_for_download_completion return successfully and the callback never gets called? If the latter, I think this works as designed. Or at least I think this works as its always worked.

jbreams avatar May 20 '22 15:05 jbreams

The callback never gets called. Problem is we can have users hang forever on fx. await waitForUpload(), if a fatal sync error happens, such as a user writing to a realm with no subscriptions.

It gets logged, and an error is send back via callback/handler passed to realm_sync_client_set_error_handler, but the callback behind await waitForUpload is not called with an error, causing that particular call to hang.

This is less than stellar.

In dart a minimal sample would look something like:

realm.write(() {
  realm.add(ARealmObject());
});
await realm.syncSession.waitForUpload();

where realm is opened with flexible sync, but subscriptions has not been set up yet.

nielsenko avatar May 31 '22 16:05 nielsenko

Sorry, if I might be on the wrong issue here.

nielsenko avatar May 31 '22 16:05 nielsenko

No, that is exactly the use case for Kotlin as well.

cmelchior avatar May 31 '22 17:05 cmelchior

It seems to me that it is either this:

https://github.com/realm/realm-core/blob/d0bcfc0a9ee53a5678b80c08815e7ea66b082409/src/realm/object-store/sync/sync_session.cpp#L947

or this: https://github.com/realm/realm-core/blob/d0bcfc0a9ee53a5678b80c08815e7ea66b082409/src/realm/object-store/sync/sync_session.cpp#L956

That is causing the problem, we should probably just invoke the callback with some error code if no subscription is present.

nicola-cab avatar Jun 01 '22 10:06 nicola-cab

Note, it is for all errors, not just subscriptions, i.e. this also happens if the server sends a Client Reset

cmelchior avatar Jun 01 '22 10:06 cmelchior

➤ Jonathan Reams commented:

I think this basically works-as-designed or is at least a forever bug/design flaw. Unless the user has been logged out, sync sessions are never "invalid" they are always active, or about to become active, and so if you are able to access a SyncSession to request download completion notifications, then it is not invalid and requesting a notification is an okay thing to do, even if that notification is going to be processed in the distant future. Maybe there's a bug specifically in how client resets work that is causing us to not fire download completions after the client reset is complete? In core version 12.1.0 we made it so doing a write on a table that does not have a subscription will throw an error before you commit, so the specific FLX case called out here should be mitigated by that.

sync-by-unito[bot] avatar Sep 06 '22 15:09 sync-by-unito[bot]

@jbreams I agree. This particular case is no longer a problem.

nielsenko avatar Sep 06 '22 15:09 nielsenko

➤ Jonathan Reams commented:

[[email protected]] is there more to do here? Is this a duplicate of RCORE-1001?

sync-by-unito[bot] avatar Sep 26 '22 18:09 sync-by-unito[bot]

➤ Jonathan Reams commented:

It's been over a month since I asked whether this was a duplicate of RCORE-1001 and I never got a response, so I'm going to close this as a duplicate. If there's more work to do here that isn't covered by RCORE-1001, please reach out and re-open.

sync-by-unito[bot] avatar Oct 26 '22 18:10 sync-by-unito[bot]