cleanup

Author: stuqdog

src/viam/sdk/module/service.cpp

@@ -194,7 +194,9 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service {
         auto new_parent_addr = request->parent_address();
         if (parent.parent_addr_ != new_parent_addr) {
             parent.parent_addr_ = std::move(new_parent_addr);
-            parent.parent_ = RobotClient::at_local_socket(parent.parent_addr_, {0, boost::none});
+            Options opts{0, boost::none};
+            opts.set_check_every_interval(5).set_reconnect_every_interval(1);
+            parent.parent_ = RobotClient::at_local_socket(parent.parent_addr_, opts);
             parent.parent_->connect_logging();
         }
         response->set_ready(parent.module_->ready());

src/viam/sdk/robot/client.cpp

@@ -2,7 +2,6 @@
 
 #include <chrono>
 #include <cstddef>
-#include <cstdlib>
 #include <memory>
 #include <mutex>
 #include <string>
@@ -145,7 +144,7 @@ RobotClient::~RobotClient() {
 
 void RobotClient::close() {
     should_refresh_.store(false);
-    should_check_connection_ = false;
+    should_check_connection_.store(false);
 
     if (refresh_thread_.joinable()) {
         refresh_thread_.join();
@@ -238,12 +237,20 @@ void RobotClient::refresh_every() {
 };
 
 void RobotClient::check_connection() {
-    std::exception connection_error;
+    unsigned int check_every = check_every_interval_;
+    unsigned int reconnect_every = reconnect_every_interval_;
+    if (check_every == 0) {
+        check_every = reconnect_every;
+    }
+    if (check_every == 0 && reconnect_every == 0) {
+        should_check_connection_.store(false);
+    }
     bool connected(true);
     while (should_check_connection_) {
+        std::exception connection_error;
         for (int i = 0; i < 3; ++i) {
             try {
-                std::this_thread::sleep_for(std::chrono::seconds{10});
+                std::this_thread::sleep_for(std::chrono::seconds{check_every});
                 impl::client_helper(impl_, &RobotService::Stub::ResourceNames).invoke([](auto&) {
                     return;
                 });
@@ -258,9 +265,10 @@ void RobotClient::check_connection() {
         if (connected) {
             continue;
         }
-        const auto* uri = viam_channel_.uri_;
-        VIAM_SDK_LOG(error)
-            << "Lost connection to machine. Attempting to reconnect to every second";
+        const auto* uri = viam_channel_.get_channel_addr();
+        VIAM_SDK_LOG(error) << "Lost connection to machine at address " << uri << " with error "
+                            << connection_error.what() << ". Attempting to reconnect to every "
+                            << reconnect_every << "second(s)";
         viam_channel_.close();
 
         for (int i = 0; i < 3; ++i) {
@@ -274,12 +282,12 @@ void RobotClient::check_connection() {
                 connected = true;
             } catch (const std::exception& e) {
                 viam_channel_.close();
-                std::this_thread::sleep_for(std::chrono::seconds{1});
+                std::this_thread::sleep_for(std::chrono::seconds{reconnect_every});
             }
         }
         if (!connected) {
             // NOLINTNEXTLINE
-            exit(0);
+            close();
         }
     }
 }
@@ -334,6 +342,9 @@ std::shared_ptr<RobotClient> RobotClient::with_channel(ViamChannel channel,
 
     robot->should_check_connection_ = true;
 
+    robot->check_every_interval_ = options.check_every_interval();
+    robot->reconnect_every_interval_ = options.reconnect_every_interval();
+
     robot->check_connection_thread_ = std::thread{&RobotClient::check_connection, robot.get()};
 
     robot->refresh();

src/viam/sdk/robot/client.hpp

@@ -187,12 +187,12 @@ class RobotClient {
     void check_connection();
 
     std::thread refresh_thread_;
-    // CR erodkin: lots of checks in this thread, it takes 10 seconds to run, shutdown is now slow
-    // on client code
     std::thread check_connection_thread_;
     std::atomic<bool> should_refresh_;
-    bool should_check_connection_;
+    std::atomic<bool> should_check_connection_;
     std::chrono::seconds refresh_interval_;
+    unsigned int check_every_interval_;
+    unsigned int reconnect_every_interval_;
 
     ViamChannel viam_channel_;
 

src/viam/sdk/rpc/dial.cpp

@@ -203,6 +203,26 @@ void ViamChannel::close() {
     pimpl_.reset();
 }
 
+const char* ViamChannel::get_channel_addr() const {
+    return uri_;
+}
+Options& Options::set_check_every_interval(unsigned int interval) {
+    check_every_interval_ = interval;
+    return *this;
+}
+Options& Options::set_reconnect_every_interval(unsigned int interval) {
+    reconnect_every_interval_ = interval;
+    return *this;
+}
+
+unsigned int Options::check_every_interval() const {
+    return check_every_interval_;
+}
+
+unsigned int Options::reconnect_every_interval() const {
+    return reconnect_every_interval_;
+}
+
 unsigned int Options::refresh_interval() const {
     return refresh_interval_;
 }

src/viam/sdk/rpc/dial.hpp

@@ -45,10 +45,10 @@ class ViamChannel {
 
     void close();
 
-    // NOLINTNEXTLINE
-    const char* uri_;
+    const char* get_channel_addr() const;
 
    private:
+    const char* uri_;
     struct impl;
 
     std::shared_ptr<GrpcChannel> channel_;
@@ -120,12 +120,32 @@ class Options {
         : refresh_interval_(std::move(refresh_interval)), dial_options_(std::move(dial_options)) {}
 
     unsigned int refresh_interval() const;
+    unsigned int check_every_interval() const;
+    unsigned int reconnect_every_interval() const;
+
+    /// @brief Sets the frequency (in seconds) to verify connectivity
+    Options& set_check_every_interval(unsigned int interval);
+
+    /// @brief Sets the frequency (in seconds) to attempt to reconnect when connectivity is lost
+    Options& set_reconnect_every_interval(unsigned int interval);
     const boost::optional<DialOptions>& dial_options() const;
 
    private:
     /// @brief How often to refresh the status/parts of the robot, in seconds. If set to 0, the
     /// robot will not automatically refresh.
     unsigned int refresh_interval_;
+
+    /// @brief How often to verify connectivity to the robot, in seconds. If set to 0, will not
+    /// check, will default to the `reconnect_every_interval_` value. Defaults to 0.
+    /// @note Setting to a non-zero value is useful in modules but may result in delays shutting
+    /// down client code
+    unsigned int check_every_interval_ = 0;
+
+    /// @brief How often to attempt to reconnect to the robot when disconnected. If set to 0,
+    /// will not attempt to reconnect. Defaults to 0.
+    /// @note Setting to a non-zero value is useful in modules but may result in delays shutting
+    /// down client code
+    unsigned int reconnect_every_interval_ = 0;
     boost::optional<DialOptions> dial_options_;
 };