Skip to content

Commit 209b5f5

Browse files
cursoragenturrsk
andcommitted
test: add regression tests for reconnect-thread blocking destructor
TCPSocketTest.setup_interruptible_by_close (test_tcp_socket.cpp): Unit test that runs without INTEGRATION_TESTS. Starts TCPSocket::setup() with max_num_tries=0 and a 5 s reconnection_timeout against a non-listening port, then calls close() from the main thread and asserts the background thread joins within 2 s. Directly exercises the interruptible-sleep fix. RTDEClientTest.destructor_not_blocked_by_stuck_reconnect_thread (test_rtde_client.cpp): Integration-level test using the existing RTDEServer fake. Initialises an RTDEClient with reconnection_timeout=5 s, drops the fake server to trigger the reconnect thread, then asserts ~RTDEClient() completes in < 2 s. The test skips gracefully when the fake server cannot complete the RTDE handshake within the socket read timeout (environment-dependent timing); in that case TCPSocketTest.setup_interruptible_by_close provides full coverage of the underlying fix. Co-authored-by: Rune Søe-Knudsen <urrsk@users.noreply.github.com>
1 parent bd62594 commit 209b5f5

2 files changed

Lines changed: 114 additions & 0 deletions

File tree

tests/test_rtde_client.cpp

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -812,6 +812,80 @@ TEST_F(RTDEClientTest, test_initialization)
812812
EXPECT_GE(std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count(), 20);
813813
}
814814

815+
// Regression test for the bug where ~RTDEClient() could block indefinitely when
816+
// the reconnect thread was sleeping inside TCPSocket::setup()'s between-attempt
817+
// sleep. Fixed by: (1) calling disconnect() before joining reconnecting_thread_
818+
// in ~RTDEClient(), and (2) making TCPSocket::setup()'s sleep interruptible by
819+
// checking for SocketState::Closed every 100 ms.
820+
//
821+
// See also TCPSocketTest.setup_interruptible_by_close in test_tcp_socket.cpp
822+
// for a lower-level unit test of the same fix that runs without INTEGRATION_TESTS.
823+
TEST_F(RTDEClientTest, destructor_not_blocked_by_stuck_reconnect_thread)
824+
{
825+
// Use a large reconnection timeout so that the blocking window is clearly
826+
// observable if the fix is absent (5 s sleep > 2 s assertion threshold).
827+
const std::chrono::milliseconds large_reconnect_timeout(5000);
828+
829+
auto fake_rtde_server = std::make_unique<RTDEServer>(g_FAKE_RTDE_PORT);
830+
// Skip the bootup-timestamp check inside isRobotBooted().
831+
fake_rtde_server->setStartTime(std::chrono::steady_clock::now() - std::chrono::seconds(52));
832+
833+
client_.reset(new rtde_interface::RTDEClient("localhost", notifier_, resources_output_recipe_,
834+
resources_input_recipe_, 100, false, g_FAKE_RTDE_PORT));
835+
// Attempt init up to 10 times with a short between-attempt sleep to ensure
836+
// the RTDE handshake succeeds even in environments where the fake server's
837+
// response arrives slightly after the 1-second socket read timeout.
838+
bool initialized = false;
839+
for (int attempt = 0; attempt < 10 && !initialized; ++attempt)
840+
{
841+
try
842+
{
843+
// max_connection_attempts=0 (unlimited): TCPSocket::setup() sleeps
844+
// large_reconnect_timeout between every failed connect attempt once the
845+
// server is gone. Use a short initialization_timeout for fast retries.
846+
client_->init(0, large_reconnect_timeout, 1, std::chrono::milliseconds(50));
847+
initialized = true;
848+
}
849+
catch (const UrException&)
850+
{
851+
// Recreate the client on each retry to start from a clean state.
852+
client_.reset(new rtde_interface::RTDEClient("localhost", notifier_, resources_output_recipe_,
853+
resources_input_recipe_, 100, false, g_FAKE_RTDE_PORT));
854+
}
855+
}
856+
if (!initialized)
857+
{
858+
GTEST_SKIP() << "Could not initialize RTDEClient with the fake server after 10 attempts; "
859+
"this test requires a reliably responding RTDE server. "
860+
"The TCPSocket-level regression test (TCPSocketTest.setup_interruptible_by_close) "
861+
"verifies the underlying fix without a robot.";
862+
}
863+
864+
// start(true) arms the reconnect callback via the background read thread.
865+
client_->start(true);
866+
867+
// Drop the server — the background read thread detects the connection loss,
868+
// calls reconnectCallback(), which launches reconnecting_thread_. That thread
869+
// enters setupCommunication() -> TCPSocket::setup() and begins sleeping
870+
// large_reconnect_timeout between retry attempts.
871+
fake_rtde_server.reset();
872+
873+
// Give the reconnect thread time to reach the sleep inside TCPSocket::setup().
874+
std::this_thread::sleep_for(std::chrono::milliseconds(500));
875+
876+
// The destructor must return quickly: disconnect() sets SocketState::Closed,
877+
// waking the sliced sleep, so the join completes in well under 2 s.
878+
// Without the fix this would block for >= large_reconnect_timeout (5 s).
879+
const auto t0 = std::chrono::steady_clock::now();
880+
client_.reset();
881+
const auto elapsed = std::chrono::steady_clock::now() - t0;
882+
883+
EXPECT_LT(elapsed, std::chrono::seconds(2))
884+
<< "RTDEClient destructor blocked for "
885+
<< std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count()
886+
<< " ms — reconnect thread was not woken by disconnect()";
887+
}
888+
815889
int main(int argc, char* argv[])
816890
{
817891
::testing::InitGoogleTest(&argc, argv);

tests/test_tcp_socket.cpp

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,46 @@ TEST_F(TCPSocketTest, connect_non_running_robot)
293293
EXPECT_LT(elapsed, std::chrono::milliseconds(7500));
294294
}
295295

296+
// Regression test for the bug where TCPSocket::setup() could block the caller
297+
// indefinitely when the sleep between retry attempts was not interruptible.
298+
//
299+
// Fixed by replacing the monolithic sleep_for(reconnection_time) in setup() with
300+
// a 100ms-sliced loop that exits early when state_ transitions to Closed (set by
301+
// a concurrent close() call, e.g. from ~RTDEClient calling disconnect() before
302+
// joining the reconnect thread).
303+
TEST_F(TCPSocketTest, setup_interruptible_by_close)
304+
{
305+
// Use a port with no listener so every connect attempt fails immediately,
306+
// sending setup() into the between-attempt sleep.
307+
const int unused_port = 12322;
308+
const std::chrono::milliseconds large_reconnect_timeout(5000);
309+
310+
Client client(unused_port, "127.0.0.1");
311+
312+
// Run setup() with unlimited retries in a background thread.
313+
std::thread setup_thread([&client, &large_reconnect_timeout]() {
314+
// max_num_tries=0 (unlimited) → setup() sleeps large_reconnect_timeout after
315+
// every failed connect attempt and never exits on its own.
316+
client.setup(0, large_reconnect_timeout);
317+
});
318+
319+
// Give the thread time to reach the between-attempt sleep inside setup().
320+
std::this_thread::sleep_for(std::chrono::milliseconds(300));
321+
322+
// close() sets state_ = Closed; the sliced sleep in setup() detects this
323+
// within 100 ms and setup() returns, allowing the thread to finish.
324+
const auto t0 = std::chrono::steady_clock::now();
325+
client.close();
326+
327+
setup_thread.join();
328+
const auto elapsed = std::chrono::steady_clock::now() - t0;
329+
330+
// Without the fix, elapsed would be >= large_reconnect_timeout (5 s).
331+
EXPECT_LT(elapsed, std::chrono::seconds(2))
332+
<< "TCPSocket::setup() was not interrupted by close() within 2 s; "
333+
"the between-attempt sleep is not interruptible";
334+
}
335+
296336
TEST_F(TCPSocketTest, test_deprecated_reconnection_time_interface)
297337
{
298338
URCL_SILENCE_DEPRECATED_BEGIN

0 commit comments

Comments
 (0)