Rework sync user handling and metadata storage #7300
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
tgoyne
force-pushed
the
tg/rework-metadata-storage
branch
7 times, most recently
from
5778fd4
to
07f0ac9
Compare
tgoyne
force-pushed
the
tg/rework-metadata-storage
branch
from
6e8a96c
to
519849b
Compare
tgoyne
force-pushed
the
tg/file-copy-race
branch
from
d09e596
to
e31fc3b
Compare
tgoyne
force-pushed
the
tg/rework-metadata-storage
branch
4 times, most recently
from
c579d7b
to
faeb506
Compare
tgoyne
force-pushed
the
tg/rework-metadata-storage
branch
from
faeb506
to
7bb0c3c
Compare
tgoyne
force-pushed
the
tg/rework-metadata-storage
branch
4 times, most recently
from
e7b82a0
to
a381d9b
Compare
|
For reference realm/realm-swift@04af045 is the changes required to update Swift for the API changes here. |
tgoyne
force-pushed
the
tg/rework-metadata-storage
branch
3 times, most recently
from
94c7d43
to
9489195
Compare
michael-wb
reviewed
Apr 8, 2024
std::string_view can already contain a null pointer and there's no reason to have a pointer to a string view.
We already have a more robust implementation of Uri splitting that should be used instead of a second implementation. Putting the redirect count on Request is more complicated than passing it directly, and testing that there are not unexpected additional requests made via checking the request count directly caught a bug which validating the redirect count did not. A few places used std::optional<std::string> where none and empty were treated identically, which means that it should be just an unwrapped string. The wrapper to heap-copy Requests did not do anything useful.
tgoyne
force-pushed
the
tg/rework-metadata-storage
branch
from
9489195
to
11a70a7
Compare
This introduces the beginning of a split between sync and app services. Object store Sync types (almost) don't depend on the `app` namespace, and can be used independently of it. The SyncUser type now implements only the functionality required internally for sync, and is backed by a UserProvider interface which allows it to request things like access token refreshes. All user management has been removed from SyncManager, and it now only owns the sync client and active sync sessions. SyncSession is mostly unchanged. `app::User` is the new equivalent of the old SyncUser type. The user management which used to be in SyncManager is now in App, which implements the UserProvider interface. Metadata storage for sync and App has been completely redesigned. The metadata store is no longer optional, and instead has an in-memory implementation that should work identically to the persistent store other than not being persistent. The interface has been reworked to enable atomic updates to the metadata store rather than relying on the filesystem mutex in SyncManager, which will be required for multiprocess sync. This required pushing significantly more logic into the metadata storage, which fortunately turned out to also simplify things in the process. The ownership relationship between `App` and `User` has been inverted, with `App` now holding a weak cache of users and `User` strongly retaining the `App`. This ensures that a `SyncConfig` now retains everything it depends on even when app caching is not used. Co-authored-by: James Stone <james.stone@mongodb.com>
We sometimes need to retry requests after doing things like fetching a new access token, so the response handlers need access to the request. This requires either copying the request or heap allocating it, and as the request body can be quite large heap allocating it is better. Previously we created them on the stack and then moved them to the heap, but as every request goes through the path where they're moved to the heap they should just start there.
tgoyne
force-pushed
the
tg/rework-metadata-storage
branch
from
11a70a7
to
84a9d03
Compare
kneth
added a commit
that referenced
this pull request
Apr 25, 2024
This was referenced Apr 25, 2024
kraenhansen
reviewed
Apr 25, 2024
| // ------------------------------------------------------------------------ | ||
| // SyncUser implementation | ||
|
|
||
| std::string user_id() const noexcept override; |
There was a problem hiding this comment.
Why not simply id ... it's already namespaced by User?
There was a problem hiding this comment.
Most of the places where user_id is used isn't in User.
jedelbo
pushed a commit
that referenced
this pull request
Apr 26, 2024
This was referenced Apr 26, 2024
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 makes a bunch of changes required by both the multiprocess sync and app services split projects because they sort of collided and had to change the same types in conflicting ways.
For the app services split, the core idea is that SyncUser is now an abstract virtual class which has just the interface required by the sync components. SyncManager is no longer involved in user management, but is now responsible for session management. When using sync without the core app services implementation, the entire
appnamespace will be disabled. The SDK will implement SyncUser to expose the SDK's users to core/sync, and instantiate a SyncManager directly.The user management functionaly that used to be split between SyncManager and App is now entirely on App. App vends
app::Userinstances, which implement the newSyncUserinterface and are roughly a drop-in replacement for the oldSyncUsertype. App continues to instantiate and own a SyncManager. Unlike before, App's cache of users is now a weak one, and insteadapp::Userinstances retain the parentApp, which makes it so that aSyncConfigremains valid without something else keeping the App alive.To support sharing users between processes, the sync metadata store has been entirely rewritten and renamed to the app metadata store to reflect that it's now part of App rather than SyncManager.
Unlike the old SyncUser, app::User is never the source of truth for the state of a user, and instead just holds a cached copy of the last-read user data. Instead there is a unidirectional data flow where all updates are made by writing to the metadata store, which then publishes updates to all live app::User instances. This simplifies the implementation and will make it so that writes to the metadata realm can update users in other processes via normal Realm change notifications. This is not yet fully implemented in this PR.
The metadata store is no longer optional, and instead there are two implementations of it: the persisted store which is backed by a Realm file, and a basic in-memory store suitable for running most tests. The in-memory store should be indistinguishable from the persisted store in any scenario that doesn't involve multiple App instances, unlike the old no metadata mode. The persisted store does not yet have the actual multi-process functionality, but it does make atomic updates to the metadata realm, unlike the old implementation which would split some updates over multiple write transactions. This may fix some bugs resulting from crashing at exactly the incorrect time. The new persisted store is also more consistent about how logged-out users are handled (they are now always in
App::all_users(), rather than only until the next launch of the process) and fixes some problems around cleaning up removed users.