Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlock when accessing user instance from within a listener #7186

Merged
merged 3 commits into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
41 changes: 22 additions & 19 deletions src/realm/object-store/sync/sync_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what this comment means anymore? I guess we're in in get_user() which implies we don't have a logged in user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this case assumes a cached user is changing to a logged in state, but in the case I'm testing against, we are updating the current logged in users auth tokens.
Comment could probably be removed or adapted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say just remove it. I don't think it was adding much context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was originally comments like this for each of the User state transitions, but they weren't maintained as the code was modified/

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()
Expand Down
20 changes: 19 additions & 1 deletion test/object-store/sync/user.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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);
Expand Down