From cb371094f2c9ccf41854169d93a7e78de6b451fb Mon Sep 17 00:00:00 2001 From: Andrew Meyer Date: Wed, 6 Dec 2023 14:15:42 +0100 Subject: [PATCH 1/3] Fix deadlock when accessing user instance from within a listener * Remove lock before calling `log_in` * Add test for deadlock --- src/realm/object-store/sync/sync_manager.cpp | 41 +++++++++++--------- test/object-store/sync/user.cpp | 20 +++++++++- 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/src/realm/object-store/sync/sync_manager.cpp b/src/realm/object-store/sync/sync_manager.cpp index 03e4b999d4c..f01ecd0bb0c 100644 --- 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 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(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(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(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> SyncManager::all_users() diff --git a/test/object-store/sync/user.cpp b/test/object-store/sync/user.cpp index f4643789bdd..132b0919003 100644 --- 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); From 82cb295c00cd9bd19fc3021b0e3144b824933fe6 Mon Sep 17 00:00:00 2001 From: Andrew Meyer Date: Thu, 7 Dec 2023 08:58:02 +0100 Subject: [PATCH 2/3] Update style and add changelog entry --- CHANGELOG.md | 3 ++- src/realm/object-store/sync/sync_manager.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39a72727195..f668f57f584 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,8 @@ ### Fixed * ([#????](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 occurs when accessing the current user from within a callback from the user listener ([#7183](https://github.com/realm/realm-core/issues/7183), since unknown) * 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: diff --git a/src/realm/object-store/sync/sync_manager.cpp b/src/realm/object-store/sync/sync_manager.cpp index f01ecd0bb0c..9c1e16fa97f 100644 --- a/src/realm/object-store/sync/sync_manager.cpp +++ b/src/realm/object-store/sync/sync_manager.cpp @@ -339,7 +339,7 @@ bool SyncManager::perform_metadata_update(util::FunctionRef SyncManager::get_user(const std::string& user_id, const std::string& refresh_token, const std::string& access_token, const std::string& device_id) { - auto user = std::shared_ptr(nullptr); + std::shared_ptr user; { util::CheckedLockGuard lock(m_user_mutex); auto it = std::find_if(m_users.begin(), m_users.end(), [&](const auto& user) { From 1e509f37b28419298bfb5c49ccf8e4096c3eea62 Mon Sep 17 00:00:00 2001 From: Andrew Meyer Date: Thu, 7 Dec 2023 13:33:57 +0100 Subject: [PATCH 3/3] Update CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kræn Hansen --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f668f57f584..c2db21971ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ ### Fixed * ([#????](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 deadlock which occurs when accessing the current user from within a callback from the user listener ([#7183](https://github.com/realm/realm-core/issues/7183), since unknown) +* 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: