Skip to content

Commit b4e4816

Browse files
urfeexcursoragent
andauthored
Fix flaky reverse_interface test (UniversalRobots#497)
Scope g_connection_mutex only around each wait_for on g_connection_condition in ReverseInterfaceTest.disconnected_callbacks_are_called. Previously the test held the mutex from the first disconnect wait through the rest of the test, which blocked the TCPServer worker in TestableReverseInterface::connectionCallback while it waited for the same mutex. The worker never returned to select/recv, so later client closes were not observed and waitForProgramState(false) timed out. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: changes are limited to test synchronization, reducing lock contention that could deadlock the server thread and cause timeouts. > > **Overview** > Fixes flakiness in `ReverseInterfaceTest.disconnected_callbacks_are_called` by **scoping `g_connection_mutex` to each `g_connection_condition.wait_for`** instead of holding a single lock across multiple disconnect/reconnect phases. > > This prevents the test thread from blocking the reverse interface’s connection/disconnection callback thread on the same mutex, so subsequent client closes are observed reliably and `waitForProgramState(false)` no longer intermittently times out. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 03272de. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 01cf607 commit b4e4816

1 file changed

Lines changed: 15 additions & 4 deletions

File tree

tests/test_reverse_interface.cpp

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -536,8 +536,11 @@ TEST_F(ReverseInterfaceTest, disconnected_callbacks_are_called)
536536
// Close the client connection
537537
client_->close();
538538
EXPECT_TRUE(waitForProgramState(1000, false));
539-
std::unique_lock<std::mutex> lk(g_connection_mutex);
540-
g_connection_condition.wait_for(lk, std::chrono::seconds(1), [&]() { return !reverse_interface_->connected.load(); });
539+
{
540+
std::unique_lock<std::mutex> lk(g_connection_mutex);
541+
g_connection_condition.wait_for(lk, std::chrono::seconds(1),
542+
[&]() { return !reverse_interface_->connected.load(); });
543+
}
541544
EXPECT_TRUE(disconnect_called_1);
542545
EXPECT_TRUE(disconnect_called_2);
543546

@@ -548,7 +551,11 @@ TEST_F(ReverseInterfaceTest, disconnected_callbacks_are_called)
548551
EXPECT_TRUE(waitForProgramState(1000, true));
549552
reverse_interface_->unregisterDisconnectionCallback(disconnection_callback_id_1);
550553
client_->close();
551-
g_connection_condition.wait_for(lk, std::chrono::seconds(1), [&]() { return !reverse_interface_->connected.load(); });
554+
{
555+
std::unique_lock<std::mutex> lk(g_connection_mutex);
556+
g_connection_condition.wait_for(lk, std::chrono::seconds(1),
557+
[&]() { return !reverse_interface_->connected.load(); });
558+
}
552559
EXPECT_TRUE(waitForProgramState(1000, false));
553560
EXPECT_FALSE(disconnect_called_1);
554561
EXPECT_TRUE(disconnect_called_2);
@@ -560,7 +567,11 @@ TEST_F(ReverseInterfaceTest, disconnected_callbacks_are_called)
560567
EXPECT_TRUE(waitForProgramState(1000, true));
561568
reverse_interface_->unregisterDisconnectionCallback(disconnection_callback_id_2);
562569
client_->close();
563-
g_connection_condition.wait_for(lk, std::chrono::seconds(1), [&]() { return !reverse_interface_->connected.load(); });
570+
{
571+
std::unique_lock<std::mutex> lk(g_connection_mutex);
572+
g_connection_condition.wait_for(lk, std::chrono::seconds(1),
573+
[&]() { return !reverse_interface_->connected.load(); });
574+
}
564575
EXPECT_TRUE(waitForProgramState(1000, false));
565576
EXPECT_FALSE(disconnect_called_1);
566577
EXPECT_FALSE(disconnect_called_2);

0 commit comments

Comments
 (0)