Skip to content

Commit 5dc121b

Browse files
authored
Fix data races in the connection state changes tests (#7871)
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.
1 parent 77ba427 commit 5dc121b

File tree

2 files changed

+10
-7
lines changed

2 files changed

+10
-7
lines changed

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
-----------
1818

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

2222
----------------------------------------------
2323

test/object-store/sync/session/connection_change_notifications.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ TEST_CASE("sync: Connection state changes", "[sync][session][connection change]"
6969

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

8989
SECTION("unregister connection change listener during callback") {
90-
uint64_t token1;
91-
std::atomic<int> listener1_call_cnt(0);
92-
std::atomic<bool> listener2_called(false);
90+
int listener1_call_cnt = 0;
9391
auto session = sync_session(
9492
user, "/connection-state-changes-3", [](auto, auto) {}, SyncSessionStopPolicy::AfterChangesUploaded);
95-
token1 = session->register_connection_change_callback(
93+
std::mutex mutex;
94+
std::unique_lock lock(mutex);
95+
uint64_t token1 = session->register_connection_change_callback(
9696
[&](SyncSession::ConnectionState, SyncSession::ConnectionState) {
97-
listener1_call_cnt = listener1_call_cnt + 1;
97+
std::lock_guard lock(mutex);
98+
++listener1_call_cnt;
9899
session->unregister_connection_change_callback(token1);
99100
});
101+
lock.unlock();
100102

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

110+
bool listener2_called = false;
108111
session->register_connection_change_callback([&](SyncSession::ConnectionState, SyncSession::ConnectionState) {
109112
listener2_called = true;
110113
});

0 commit comments

Comments
 (0)