Fix client reset callbacks not populating Realm instances sometimes #5654
Conversation
|
I have recently been thinking that the RealmConfig/SyncConfig split doesn't make sense any more, and hasn't for a long time. When we first designed it, there were two very big differences from today. The first was that sync was closed-source (including the SDK part at the time) and minimizing merge conflicts between the public and private branches was important. Splitting the sync stuff off as much as possible helped with that. The second and probably bigger one was that we were still thinking of ObjectStore as a set of modular components that SDKs would use when they found them useful, and skip if they didn't. This meant that SyncSession should be totally decoupled from Realm so that you could use one without the other (and in practice Java did use SyncSession without Realm for a while). There's a pile of design quirks remaining from this, but SyncConfig and RealmConfig being different types is the most obvious and probably easiest to fix of them. ( I haven't looked at the actual changes here yet, but I guess the point is that I don't hate the idea of passing RealmConfig to SyncSession. |
| @@ -311,16 +314,17 @@ class SyncSession : public std::enable_shared_from_this<SyncSession> { | |||
|
|
|||
| friend class realm::SyncManager; | |||
| // Called by SyncManager { | |||
| static std::shared_ptr<SyncSession> create(_impl::SyncClient& client, std::shared_ptr<DB> db, SyncConfig config, | |||
| static std::shared_ptr<SyncSession> create(_impl::SyncClient& client, std::shared_ptr<DB> db, RealmConfig config, | |||
There was a problem hiding this comment.
Everything in the chain SyncSession::get_session() to SyncSession::SyncSession() should probably be passing const RealmConfig&. RealmConfig is sufficiently large and complicated that even moving it is not really trivial.
| config.mode = session_config.sync_config->client_resync_mode; | ||
| config.notify_after_client_reset = [config = copy_config](std::string local_path, VersionID previous_version, | ||
| bool did_recover) { | ||
| if (config.sync_config->notify_after_client_reset) { |
There was a problem hiding this comment.
Can't this condition be pulled out of the lambda?
| @@ -235,7 +235,7 @@ SyncSession::handle_refresh(const std::shared_ptr<SyncSession>& session) | |||
| }; | |||
| } | |||
|
|
|||
| SyncSession::SyncSession(SyncClient& client, std::shared_ptr<DB> db, SyncConfig config, SyncManager* sync_manager) | |||
| SyncSession::SyncSession(SyncClient& client, std::shared_ptr<DB> db, RealmConfig config, SyncManager* sync_manager) | |||
There was a problem hiding this comment.
This needs to set scheduler and audit_config to nullptr on m_config. Trying to use the explicitly set scheduler will break since we're not obtaining the Realm instance while inside that scheduler. Audit stuff might not break but we don't really want to be initializing that while performing a client reset regardless.
ironage
force-pushed
the
js/notify-resets
branch
from
6698e46 to
0d2655e
Compare
As reported by @LaPeste, the client reset callbacks would be called with null Realm instances if the call to
RealmCoordinator ::get_existing_coordinator()returned a nullptr. This can happen if the sync session is running a client reset operation while all references to the Realm have been released. To fix this, we need to get or create a coordinator and the way to do that is to pass in theRealmConfig. This is not something that the sync session had, so I added it in place of the SyncConfig that was already there. It is a bit of a heavy handed solution, but a better way wasn't obvious to me.☑️ ToDos