Merge pull request #7186 from realm/andrew/double-login

Fix deadlock when accessing user instance from within a listener

CHANGELOG.md

@@ -7,7 +7,8 @@
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
-* Fixed several causes of "decryption failed" exceptions that could happen when opening multiple encrypted Realm files in the same process while using Apple/linux and storing the Realms on an exFAT file system. ([#7156](https://github.com/realm/realm-core/issues/7156), since the beginning) 
+* Fixed several causes of "decryption failed" exceptions that could happen when opening multiple encrypted Realm files in the same process while using Apple/linux and storing the Realms on an exFAT file system. ([#7156](https://github.com/realm/realm-core/issues/7156), since the beginning)
+* Fixed deadlock which occurred when accessing the current user from the `App` from within a callback from the `User` listener ([#7183](https://github.com/realm/realm-core/issues/7183), since v13.21.0)
 * Update existing std exceptions thrown by the Sync Client to use Realm exceptions. ([#6255](https://github.com/realm/realm-core/issues/6255), since v10.2.0)
 * Having a class name of length 57 would make client reset crash as a limit of 56 was wrongly enforced (57 is the correct limit) ([#7176](https://github.com/realm/realm-core/issues/7176), since v10.0.0)
 * Automatic client reset recovery on flexible sync Realms would apply recovered changes in multiple write transactions, releasing the write lock in between. This had several observable negative effects:

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;
+    std::shared_ptr<SyncUser> user;
+    {
+        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()

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);