Allow disconnect to stop the G5 dashboard welcome read

src/ur/dashboard_client_implementation_g5.cpp

@@ -26,6 +26,7 @@
 // ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 // POSSIBILITY OF SUCH DAMAGE.
 
+#include <chrono>
 #include <filesystem>
 #include <regex>
 #include <sstream>
@@ -207,42 +208,70 @@ bool DashboardClientImplG5::connect(const size_t max_num_tries, const std::chron
     URCL_LOG_ERROR("%s", "Socket is already connected. Refusing to reconnect.");
     return false;
   }
-  bool ret_val = false;
-
   timeval configured_tv = getConfiguredReceiveTimeout();
-
-  while (!ret_val)
+  try
   {
-    timeval tv;
-    // The first read after connection can take more time.
-    tv.tv_sec = 10;
-    tv.tv_usec = 0;
-    TCPSocket::setReceiveTimeout(tv);
-    try
+    if (TCPSocket::connect(host_, port_, max_num_tries, reconnection_time))
     {
-      if (TCPSocket::connect(host_, port_, max_num_tries, reconnection_time))
+      // The welcome receive must remain interruptible by disconnect(). A ten-second
+      // blocking recv can outlive a shutdown request even after its socket is closed.
+      timeval poll_tv;
+      poll_tv.tv_sec = 0;
+      poll_tv.tv_usec = 100000;
+      TCPSocket::setReceiveTimeout(poll_tv);
+      const timeval welcome_tv = { 10, 0 };
+      std::string welcome;
+      auto deadline = std::chrono::steady_clock::now() + 10s;
+      while (!isStopRequested())
       {
-        URCL_LOG_INFO("%s", read().c_str());
-        ret_val = true;
+        char character;
+        size_t read_chars = 0;
+        if (TCPSocket::read(reinterpret_cast<uint8_t*>(&character), 1, read_chars))
+        {
+          welcome.push_back(character);
+          if (character == '\n')
+          {
+            break;
+          }
+          deadline = std::chrono::steady_clock::now() + 10s;
+        }
+        else if (getState() != comm::SocketState::Connected || std::chrono::steady_clock::now() >= deadline)
+        {
+          throw TimeoutException("Did not receive welcome message from dashboard server in time.", welcome_tv);
+        }
       }
-      else
+      if (isStopRequested())
       {
+        TCPSocket::setReceiveTimeout(configured_tv);
         return false;
       }
+      URCL_LOG_INFO("%s", welcome.c_str());
     }
-    catch (const TimeoutException&)
+    else
     {
-      URCL_LOG_WARN("Did not receive dashboard bootup message although connection was established. This should not "
-                    "happen, please contact the package maintainers. Retrying anyway...");
+      TCPSocket::setReceiveTimeout(configured_tv);
+      return false;
     }
   }
+  catch (const TimeoutException& e)
+  {
+    if (isStopRequested())
+    {
+      TCPSocket::setReceiveTimeout(configured_tv);
+      return false;
+    }
+    URCL_LOG_ERROR("%s", e.what());
+    disconnect();
+    TCPSocket::setReceiveTimeout(configured_tv);
+    return false;
+  }
 
   // Reset read timeout to configured socket timeout
   TCPSocket::setReceiveTimeout(configured_tv);
 
   polyscope_version_ = queryPolyScopeVersion();
 
-  return ret_val;
+  return true;
 }
 
 void DashboardClientImplG5::disconnect()

tests/CMakeLists.txt

@@ -175,6 +175,10 @@ add_executable(dashboard_client_mock_tests test_dashboard_client.cpp)
 target_link_libraries(dashboard_client_mock_tests PRIVATE ur_client_library::urcl GTest::gmock_main)
 gtest_add_tests(TARGET      dashboard_client_mock_tests
 )
+set_tests_properties(DashboardClientWelcomeTest.disconnect_interrupts_silent_welcome
+                     PROPERTIES TIMEOUT 20 RUN_SERIAL TRUE)
+set_tests_properties(DashboardClientWelcomeTest.welcome_timeout_allows_external_retry
+                     PROPERTIES TIMEOUT 30 RUN_SERIAL TRUE)
 
 add_executable(primary_parser_tests test_primary_parser.cpp)
 target_link_libraries(primary_parser_tests PRIVATE ur_client_library::urcl GTest::gtest_main)

tests/test_dashboard_client.cpp

@@ -36,7 +36,11 @@
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
+#include <chrono>
+#include <exception>
 #include <memory>
+#include <thread>
+#include "test_utils.h"
 
 using namespace urcl;
 
@@ -402,6 +406,111 @@ TEST_F(DashboardClientTest, X_program_api)
   EXPECT_EQ(prog.programState, "FINAL");
 }
 
+// A dashboard peer may accept TCP but never send the welcome line. A deliberate
+// disconnect must end that receive without waiting for its ten-second timeout.
+TEST(DashboardClientWelcomeTest, disconnect_interrupts_silent_welcome)
+{
+  TestableTcpServer server(29999);
+  server.start();
+  DashboardClientImplG5 client("127.0.0.1");
+  bool connected = false;
+  std::exception_ptr connection_error;
+  std::thread connection([&]() {
+    try
+    {
+      connected = client.connect(1);
+    }
+    catch (...)
+    {
+      connection_error = std::current_exception();
+    }
+  });
+
+  const bool accepted = server.waitForConnectionCallback(5000);
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
+  const auto started = std::chrono::steady_clock::now();
+  client.disconnect();
+  connection.join();
+  const auto elapsed = std::chrono::steady_clock::now() - started;
+
+  ASSERT_TRUE(accepted) << "The loopback dashboard server did not accept the connection";
+  if (connection_error)
+  {
+    std::rethrow_exception(connection_error);
+  }
+  EXPECT_FALSE(connected);
+  EXPECT_LT(elapsed, std::chrono::seconds(3));
+}
+
+// A missing welcome is a failed connection attempt, not a connected socket
+// that leaves every later external connect() unable to try again.
+TEST(DashboardClientWelcomeTest, welcome_timeout_allows_external_retry)
+{
+  TestableTcpServer server(29999);
+  server.start();
+  DashboardClientImplG5 client("127.0.0.1");
+
+  const auto first_started = std::chrono::steady_clock::now();
+  const bool first_connected = client.connect(1);
+  const auto first_elapsed = std::chrono::steady_clock::now() - first_started;
+  const bool first_accepted = server.waitForConnectionCallback(1000);
+  const bool first_disconnected = server.waitForDisconnectionCallback(1000);
+
+  bool second_connected = false;
+  std::exception_ptr connection_error;
+  std::thread second_attempt([&]() {
+    try
+    {
+      second_connected = client.connect(1);
+    }
+    catch (...)
+    {
+      connection_error = std::current_exception();
+    }
+  });
+
+  const bool second_accepted = server.waitForConnectionCallback(1000);
+  bool greeting_sent = false;
+  bool version_requested = false;
+  bool version_sent = false;
+  if (second_accepted)
+  {
+    const std::string greeting = "Connected: Universal Robots Dashboard Server\n";
+    size_t written = 0;
+    greeting_sent = server.write(reinterpret_cast<const uint8_t*>(greeting.data()), greeting.size(), written) &&
+                    written == greeting.size();
+    if (greeting_sent)
+    {
+      version_requested = server.waitForMessageCallback(2000);
+    }
+    if (version_requested)
+    {
+      const std::string version = "URSoftware 5.12.0.1 (validation)\n";
+      version_sent = server.write(reinterpret_cast<const uint8_t*>(version.data()), version.size(), written) &&
+                     written == version.size();
+    }
+  }
+  if (!second_accepted || !greeting_sent || !version_requested || !version_sent)
+  {
+    client.disconnect();
+  }
+  second_attempt.join();
+
+  EXPECT_TRUE(first_accepted);
+  EXPECT_FALSE(first_connected);
+  EXPECT_LT(first_elapsed, std::chrono::seconds(13));
+  EXPECT_TRUE(first_disconnected) << "The timed-out welcome left the socket connected";
+  EXPECT_TRUE(second_accepted) << "The next connect() never reached the dashboard server";
+  EXPECT_TRUE(greeting_sent);
+  EXPECT_TRUE(version_requested);
+  EXPECT_TRUE(version_sent);
+  if (connection_error)
+  {
+    std::rethrow_exception(connection_error);
+  }
+  EXPECT_TRUE(second_connected);
+}
+
 int main(int argc, char* argv[])
 {
   ::testing::InitGoogleTest(&argc, argv);