Add support for uploading and downloading changes with a timeout #6073
Conversation
realm/realm-library/src/objectServer/java/io/realm/SyncSession.java
Outdated
Show resolved
Hide resolved
realm/realm-library/src/objectServer/java/io/realm/SyncSession.java
Outdated
Show resolved
Hide resolved
| // session, including trying to stop it. | ||
| // In Java we cannot lock on the Session object either since it will prevent any attempt at modifying the | ||
| // lifecycle while it is in a waiting state. Thus we use a specialised mutex. | ||
| synchronized (waitForChangesMutex) { |
There was a problem hiding this comment.
Since the lock is at the Java level only, then I question the usefulness of these overloads, since the m_state_mutex will still be acquired from the call to the OS's session->wait_for_download_completion, by returning you give the user the false impression that the underlying operation has been cancelled/timed out which is clearly not the case. Imagine this use case
SyncSession sync;
sync.downloadAllServerChanges(1, TimeUnit.MINUTES)
// do some commits
sync.uploadAllServerChanges(1, TimeUnit.MINUTES);
// by returning here you have no guarantee that the actcual upload was triggered (at OS level), since we might still be holding the lock at the C++ level, from the downloadAll callThere was a problem hiding this comment.
We are using the asynchronous methods in the C++ layer, so no locks are actually being held. We are just using the Java lock to wait for the callback to happen. The reason for this that is that it is otherwise impossible to interrupt the Java thread at all, which would be highly undesirable.
IMO the semantics we have is that if downloadAllServerChanges completes, then changes have been downloaded. If it fails or times out, the behavior is undefined, i.e. it doesn't mean that downloading is canceled, it just means it didn't complete in time. I think that is acceptable. Behaving otherwise indicate a level of control over the syncing process that we don't offer (as everything happens more or less automatically).
There was a problem hiding this comment.
I have updated the JavaDoc to indicate that downloading/uploading might still happen.
# Conflicts: # CHANGELOG.md # realm/realm-library/src/objectServer/java/io/realm/SyncSession.java
|
Updated this PR as we had new use cases appear for it. It has been updated to support timeouts for downloading initial data and is ready for review. |
realm/realm-library/src/objectServer/java/io/realm/SyncSession.java
Outdated
Show resolved
Hide resolved
realm/realm-library/src/objectServer/java/io/realm/SyncSession.java
Outdated
Show resolved
Hide resolved
realm/realm-library/src/objectServer/java/io/realm/SyncSession.java
Outdated
Show resolved
Hide resolved
realm/realm-library/src/objectServer/java/io/realm/SyncSession.java
Outdated
Show resolved
Hide resolved
| bgRealm.createObject(AllTypes.class); | ||
| } | ||
| }) | ||
| .waitForInitialRemoteData(1, TimeUnit.MILLISECONDS) |
There was a problem hiding this comment.
NANOSECONDS? to be completely sure 🤓
# Conflicts: # CHANGELOG.md
Closes #6247
This PR adds two convenience methods overrides for
SyncSession.downloadAllServerChanges()andSyncSession.uploadAllLocalChanges()so they will now terminate if a timeout is hit. This will make it a lot easier to implement sync logic that is guaranteed to proceed after a given period without having to resort to external synchronizationThis PR also add support for
SyncConfiguration.waitForInitialRemoteData(timeout, unit)as it uses the above helper methods.