This repository was archived by the owner on Nov 8, 2021. It is now read-only.
Fix some async open issues #806
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we open the Realm on the background thread, rather than bind it to that thread, leave it in an unbound state and pass it to the callback as a ThreadSafeReference<Realm>, which the SDK can then bind to a thread at a later time. This works around an issue in the libuv version of EventLoopSignal where creating it on a background thread doesn't work, and makes async open work better with Realm caching without requiring that the SDK reopen the Realm on the destination thread, which is a potentially somewhat expensive operation.
The state directory needs to be Realm-specific and not a global directory shared by all Realms, so the previous version broke if more than one Realm tried to use it at a time.
cmelchior
approved these changes
Jun 20, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This includes changes needed to get the realm-js and realm-cocoa tests passing using the new async open, and fixes a bug which hits Cocoa which wasn't caught by those tests.
AsyncOpenTask now takes more control over its lifetime: the callback holds a strong reference to the task until it's called so that retaining the AsyncOpenTask externally is optional, and cleans itself up after calling the callback passed to
start()so that the SDK code doesn't have to worry about that. Some thread-safety problems around usingm_sessionfrom multiple threads have been fixed by usingAtomicSharedPtr<>, and callingcancel()while it happens to be processing the callback will no longer crash.The state dir for async open/resync is now a directory named after the Realm rather than a single shared directory, since it turns out that's what the sync code expects. Sharing a single directory resulted in async opening more than one Realm at a time breaking.
The AsyncOpenTask callback is now passed a
ThreadSafeReference<Realm>rather thanstd::shared_ptr<Realm>. The TSR can be resolved to an actual Realm by passing it toRealm::get_shared_realm(). This avoids creating the EventLoopSignal for the Realm on the background thread (which doesn't work with the libuv implementation because libuv event loops aren't thread-safe), and lets the JS SDK skip closing and re-opening the Realm back on the main thread. This also happened to reveal that the background-opened Realm wasn't really in a valid state, so a few fixes were needed there.Async open failed to check that the history schema for existing Realm files was compatible before creating the sync session, which resulted in an exception being thrown on the sync client thread that did not get funneled back to the appropraite place.