Skip to content

Commit

Permalink
Fix data races in the connection state changes tests
Browse files Browse the repository at this point in the history
The assignment to token1 on the main thread and the read of it on the sync
worker thread didn't have any intervening synchronization and in theory the
callback could read it before it's actually written. Fixing this requires
adding some locking.

Conversely, listener2 doesn't actually need to be atomic since the second
callback should only ever be invokved synchronously inside log_out(), and if
it's called at some other time that's a bug.

It doesn't matter here, but `listener1_call_cnt = listener1_call_cnt + 1` is a
nonatomic increment that will drop updates if it happens on multiple threads at
once, while `++` will not.
  • Loading branch information
tgoyne committed Jul 10, 2024
1 parent 77ba427 commit 9bfe056
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 7 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
-----------

### Internals
* None.
* Fix a thread sanitizer failure in the "unregister connection change listener during callback" test ([PR #7871](https://github.com/realm/realm-core/pull/7871)).

----------------------------------------------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ TEST_CASE("sync: Connection state changes", "[sync][session][connection change]"

auto token1 = session->register_connection_change_callback(
[&](SyncSession::ConnectionState, SyncSession::ConnectionState) {
listener1_call_cnt = listener1_call_cnt + 1;
++listener1_call_cnt;
});
session->unregister_connection_change_callback(token1);
// One call may have been in progress when unregistered
Expand All @@ -87,16 +87,18 @@ TEST_CASE("sync: Connection state changes", "[sync][session][connection change]"
}

SECTION("unregister connection change listener during callback") {
uint64_t token1;
std::atomic<int> listener1_call_cnt(0);
std::atomic<bool> listener2_called(false);
int listener1_call_cnt = 0;
auto session = sync_session(
user, "/connection-state-changes-3", [](auto, auto) {}, SyncSessionStopPolicy::AfterChangesUploaded);
token1 = session->register_connection_change_callback(
std::mutex mutex;
std::unique_lock lock(mutex);
uint64_t token1 = session->register_connection_change_callback(
[&](SyncSession::ConnectionState, SyncSession::ConnectionState) {
listener1_call_cnt = listener1_call_cnt + 1;
std::lock_guard lock(mutex);
++listener1_call_cnt;
session->unregister_connection_change_callback(token1);
});
lock.unlock();

EventLoop::main().run_until([&] {
return sessions_are_active(*session);
Expand All @@ -105,6 +107,7 @@ TEST_CASE("sync: Connection state changes", "[sync][session][connection change]"
return sessions_are_connected(*session);
});

bool listener2_called = false;
session->register_connection_change_callback([&](SyncSession::ConnectionState, SyncSession::ConnectionState) {
listener2_called = true;
});
Expand Down

0 comments on commit 9bfe056

Please sign in to comment.