Fix deadlock when accessing user instance from within a listener

* Remove lock before calling `log_in` * Add test for deadlock
realm · Dec 6, 2023 · 3058030 · 3058030
1 parent 41dc9c7
commit 3058030
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 20 deletions.
diff --git a/src/realm/object-store/sync/sync_manager.cpp b/src/realm/object-store/sync/sync_manager.cpp
@@ -339,28 +339,31 @@ bool SyncManager::perform_metadata_update(util::FunctionRef<void(SyncMetadataMan
 std::shared_ptr<SyncUser> SyncManager::get_user(const std::string& user_id, const std::string& refresh_token,
                                                 const std::string& access_token, const std::string& device_id)
 {
-    util::CheckedLockGuard lock(m_user_mutex);
-    auto it = std::find_if(m_users.begin(), m_users.end(), [&](const auto& user) {
-        return user->identity() == user_id && user->state() != SyncUser::State::Removed;
-    });
-    if (it == m_users.end()) {
-        // No existing user.
-        auto new_user = std::make_shared<SyncUser>(refresh_token, user_id, access_token, device_id, this);
-        m_users.emplace(m_users.begin(), new_user);
-        {
-            util::CheckedLockGuard lock(m_file_system_mutex);
-            // m_current_user is normally set very indirectly via the metadata manger
-            if (!m_metadata_manager)
-                m_current_user = new_user;
+    auto user = std::shared_ptr<SyncUser>(nullptr);
+    {
+        util::CheckedLockGuard lock(m_user_mutex);
+        auto it = std::find_if(m_users.begin(), m_users.end(), [&](const auto& user) {
+            return user->identity() == user_id && user->state() != SyncUser::State::Removed;
+        });
+        if (it == m_users.end()) {
+            // No existing user.
+            auto new_user = std::make_shared<SyncUser>(refresh_token, user_id, access_token, device_id, this);
+            m_users.emplace(m_users.begin(), new_user);
+            {
+                util::CheckedLockGuard lock(m_file_system_mutex);
+                // m_current_user is normally set very indirectly via the metadata manger
+                if (!m_metadata_manager)
+                    m_current_user = new_user;
+            }
+            return new_user;
         }
-        return new_user;
-    }
-    else { // LoggedOut => LoggedIn
-        auto user = *it;
+
+        // LoggedOut => LoggedIn
+        user = *it;
         REALM_ASSERT(user->state() != SyncUser::State::Removed);
-        user->log_in(access_token, refresh_token);
-        return user;
     }
+    user->log_in(access_token, refresh_token);
+    return user;
 }
 
 std::vector<std::shared_ptr<SyncUser>> SyncManager::all_users()

diff --git a/test/object-store/sync/user.cpp b/test/object-store/sync/user.cpp
@@ -112,7 +112,7 @@ TEST_CASE("sync_user: update state and tokens", "[sync][user]") {
     user->invalidate();
 }
 
-TEST_CASE("sync_user: SyncManager `get_existing_logged_in_user()` API", "[sync][user]") {
+TEST_CASE("sync_user: SyncManager get_existing_logged_in_user() API", "[sync][user]") {
     TestSyncManager init_sync_manager(SyncManager::MetadataMode::NoMetadata);
     auto sync_manager = init_sync_manager.app()->sync_manager();
     const std::string identity = "sync_test_identity";
@@ -124,6 +124,24 @@ TEST_CASE("sync_user: SyncManager `get_existing_logged_in_user()` API", "[sync][
         REQUIRE(!user);
     }
 
+    SECTION("can get logged-in user from notification") {
+        auto first = sync_manager->get_user(identity, refresh_token, access_token, dummy_device_id);
+        REQUIRE(first->identity() == identity);
+        REQUIRE(first->state() == SyncUser::State::LoggedIn);
+        REQUIRE(first->device_id() == dummy_device_id);
+        bool notification_fired = false;
+        auto sub_token = first->subscribe([&](const SyncUser& user) {
+            auto current_user = sync_manager->get_current_user();
+            REQUIRE(current_user->identity() == identity);
+            REQUIRE(current_user->identity() == user.identity());
+            notification_fired = true;
+        });
+
+        auto second = sync_manager->get_user(identity, refresh_token, access_token, dummy_device_id);
+        second->unsubscribe(sub_token);
+        REQUIRE(notification_fired);
+    }
+
     SECTION("properly returns an existing logged-in user") {
         auto first = sync_manager->get_user(identity, refresh_token, access_token, dummy_device_id);
         REQUIRE(first->identity() == identity);