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 race to unregister sync session #5886

Merged
merged 12 commits into from
Sep 30, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* Fix a data race reported by thread sanitizer when preparing to deliver change notifications. This probably did not cause observable problems in practice ([PR #5892](https://github.com/realm/realm-core/pull/5892) since 12.7.0).
* Changed the behaviour of creating a collection of non-nullable Mixed type to throw a descriptive error message rather than asserting in debug mode. ([#5894](https://github.com/realm/realm-core/issues/5894), since the introduction of the Mixed type).
* Fix a use-after-free when a sync session is closed and the app is destroyed at the same time ([#5752](https://github.com/realm/realm-core/issues/5752), since v11.5.2).

### Breaking changes
* None.
Expand Down
8 changes: 0 additions & 8 deletions src/realm/object-store/impl/realm_coordinator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -329,14 +329,6 @@ std::shared_ptr<AsyncOpenTask> RealmCoordinator::get_synchronized_realm(Realm::C
return std::make_shared<AsyncOpenTask>(shared_from_this(), m_sync_session);
}

void RealmCoordinator::create_session(const Realm::Config& config)
{
REALM_ASSERT(config.sync_config);
util::CheckedLockGuard lock(m_realm_mutex);
set_config(config);
open_db();
}

#endif

REALM_NOINLINE void realm::_impl::translate_file_exception(StringData path, bool immutable)
Expand Down
5 changes: 0 additions & 5 deletions src/realm/object-store/impl/realm_coordinator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,6 @@ class RealmCoordinator : public std::enable_shared_from_this<RealmCoordinator> {
std::shared_ptr<AsyncOpenTask> get_synchronized_realm(Realm::Config config)
REQUIRES(!m_realm_mutex, !m_schema_cache_mutex);

// Creates the underlying sync session if it doesn't already exists.
// This is also created as part of opening a Realm, so only use this
// method if the session needs to exist before the Realm does.
void create_session(const Realm::Config& config) REQUIRES(!m_realm_mutex, !m_schema_cache_mutex);

std::shared_ptr<SyncSession> sync_session() REQUIRES(!m_realm_mutex)
{
util::CheckedLockGuard lock(m_realm_mutex);
Expand Down
10 changes: 1 addition & 9 deletions src/realm/object-store/sync/sync_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ void SyncManager::delete_user(const std::string& user_id)
}
}

SyncManager::~SyncManager()
SyncManager::~SyncManager() NO_THREAD_SAFETY_ANALYSIS
{
// Grab a vector of the current sessions under a lock so we can shut them down. We have to make a copy because
// session->shutdown_and_wait() will modify the m_sessions map.
Expand All @@ -508,14 +508,6 @@ SyncManager::~SyncManager()
session->detach_from_sync_manager();
}

// At this point we should have drained all the sessions.
#ifdef REALM_DEBUG
{
util::CheckedLockGuard lk(m_session_mutex);
REALM_ASSERT(m_sessions.empty());
}
#endif

{
util::CheckedLockGuard lk(m_user_mutex);
for (auto& user : m_users) {
Expand Down
20 changes: 16 additions & 4 deletions src/realm/object-store/sync/sync_session.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,16 @@ void SyncSession::become_inactive(util::CheckedUniqueLock lock, std::error_code
std::swap(waits, m_completion_callbacks);

m_session = nullptr;
auto& sync_manager = *m_sync_manager;
std::shared_ptr<SyncManager> sync_manager;
if (m_sync_manager) {
// This method may be called from the SyncManager's destructor. In this case, using 'shared_from_this()' will
// throw 'std::bad_weak_ptr' so we use a weak_ptr as intermediate.
sync_manager = m_sync_manager->weak_from_this().lock();
}
m_state_mutex.unlock(lock);
sync_manager.unregister_session(m_db->get_path());
if (sync_manager) {
sync_manager->unregister_session(m_db->get_path());
}

// Send notifications after releasing the lock to prevent deadlocks in the callback.
if (old_state != new_state) {
Expand Down Expand Up @@ -963,9 +970,14 @@ void SyncSession::close(util::CheckedUniqueLock lock)
m_state_mutex.unlock(lock);
break;
case State::Inactive: {
auto& sync_manager = *m_sync_manager;
std::shared_ptr<SyncManager> sync_manager;
if (m_sync_manager) {
sync_manager = m_sync_manager->weak_from_this().lock();
}
m_state_mutex.unlock(lock);
sync_manager.unregister_session(m_db->get_path());
if (sync_manager) {
sync_manager->unregister_session(m_db->get_path());
}
break;
}
case State::WaitingForAccessToken:
Expand Down
4 changes: 3 additions & 1 deletion src/realm/object-store/sync/sync_session.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ class SyncSession : public std::enable_shared_from_this<SyncSession> {
// longer be open on behalf of it.
void shutdown_and_wait() REQUIRES(!m_state_mutex, !m_connection_state_mutex);

// DO NOT CALL OUTSIDE OF TESTING CODE.
void detach_from_sync_manager() REQUIRES(!m_state_mutex, !m_connection_state_mutex);

// The access token needs to periodically be refreshed and this is how to
// let the sync session know to update it's internal copy.
void update_access_token(const std::string& signed_token) REQUIRES(!m_state_mutex, !m_config_mutex);
Expand Down Expand Up @@ -344,7 +347,6 @@ class SyncSession : public std::enable_shared_from_this<SyncSession> {
void create_sync_session() REQUIRES(m_state_mutex, !m_config_mutex);
void did_drop_external_reference()
REQUIRES(!m_state_mutex, !m_config_mutex, !m_external_reference_mutex, !m_connection_state_mutex);
void detach_from_sync_manager() REQUIRES(!m_state_mutex, !m_connection_state_mutex);
void close(util::CheckedUniqueLock) RELEASE(m_state_mutex) REQUIRES(!m_config_mutex, !m_connection_state_mutex);

void become_active() REQUIRES(m_state_mutex, !m_config_mutex);
Expand Down
7 changes: 7 additions & 0 deletions test/object-store/sync/session/session.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,13 @@ TEST_CASE("SyncSession: close() API", "[sync]") {
session->close();
REQUIRE(sessions_are_inactive(*session));
}

SECTION("Close session after it was detached from the SyncManager") {
auto session = sync_session(
user, "/test-close-after-detach", [](auto, auto) {}, SyncSessionStopPolicy::Immediately);
session->detach_from_sync_manager();
REQUIRE_NOTHROW(session->close());
}
}

TEST_CASE("SyncSession: shutdown_and_wait() API", "[sync]") {
Expand Down