RCORE-2172 Fix race condition in "unregister connection change listener" test while listener is being unregistered #7824
Conversation
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1174Details
💛 - Coveralls |
| session->register_connection_change_callback([&](SyncSession::ConnectionState, SyncSession::ConnectionState) { | ||
| listener2_called = true; | ||
| }); | ||
|
|
||
| user->log_out(); | ||
| REQUIRE(sessions_are_disconnected(*session)); | ||
| REQUIRE(listener1_called == false); | ||
| REQUIRE(listener1_call_cnt == 1); |
There was a problem hiding this comment.
The way I read this test is that it is specifically supposed to check that an unregistered connection change callback is never called. But this changes the test to not test that anymore, am I missing something?
There was a problem hiding this comment.
There are actually two cases that should be tested here and maybe this should be another SECTION - there is the case where the callback is unregistered when the callback is called (was not explicitly tested before); and then there is the case of registering and then unregistering a callback and (hopefully) a connection change event has not occurred before the callback can be unregistered.
There is a tiny window where the callback could grab the mutex and unregister the callback while the callback is being executed. I'll update the previous test and add a second SECTION to test the unregister during callback.
| [&](SyncSession::ConnectionState, SyncSession::ConnectionState) { | ||
| listener1_called = true; | ||
| }); | ||
| session->unregister_connection_change_callback(token1); |
There was a problem hiding this comment.
I'd guess that the race is because there is spurious network connection changes between the calls to register and unregister. If that is the case, what you could do is get the count of times called after unregistering the callback, and check that it is the same after calling log_out() below.
Pull Request Test Coverage Report for Build michael.wilkersonbarker_1176Details
💛 - Coveralls |
test/object-store/sync/session/connection_change_notifications.cpp
Outdated
Show resolved
Hide resolved
test/object-store/sync/session/connection_change_notifications.cpp
Outdated
Show resolved
Hide resolved
What, How & Why?
Updated the "unregister connection change listener" so it registers a callback when the session is first created and unregisters the callback when it is called the first time. There is a call count variable that is updated every time this callback is called and verified that it is only called one time during the test.
Fixes #7823
☑️ ToDos
[ ] 📝 Changelog update[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml, if public C++ API changed