RSDK-6158 - initial connection options #376

stuqdog · 2025-02-28T20:41:46Z

No description provided.

lia-viam · 2025-03-03T14:33:12Z

src/viam/sdk/rpc/dial.cpp

 }

+void DialOptions::set_initial_connection_attempt_timeout(std::chrono::duration<float> timeout) {
+    initial_connection_attempt_timeout_ = std::move(timeout);


minor but you can remove the std::move here, a chrono::duration<T> is a T under the hood, no need to std::move a float.

I am seeing that this was also how it was done in set_timeout above, so maybe that can be a drive-by

I wonder if clang-tidy will think of it as a missed move? Personally, I always move things like these, since otherwise it feels like I'm making assumptions about implementation.

I generally err on the side of move unless clang explicitly tells me not to do so just because it's simpler and makes me less likely to miss a move that is relevant but I don't feel particularly strongly. I'm happy to go with whatever makes sense to y'all!

Ended up going with @lia-viam's suggestion just because it was the one that came in first. But again, I'm not picky and my slight inclination if anything is to keep moveing things even if it's unnecessary. So if you two land on the other approach I'm happy to change it back.

lia-viam · 2025-03-03T14:34:03Z

src/viam/sdk/rpc/dial.hpp

    bool allows_insecure_downgrade() const;
    const std::chrono::duration<float>& timeout() const;
+    int initial_connection_attempts() const;
+    const std::chrono::duration<float>& initial_connection_attempt_timeout() const;


Let's return this by value instead of ref to const; see other comment

lia-viam · 2025-03-03T14:38:01Z

src/viam/sdk/rpc/dial.cpp

+            auto connection = dial(uri, opts);
+            opts.set_timeout(timeout);
+            return connection;
+        } catch (...) {


Let's just catch const std::exception& here, a catch (...) is generally inadvisable except at application boundaries.

lia-viam · 2025-03-03T14:38:47Z

src/viam/sdk/rpc/dial.cpp

+                                               const boost::optional<DialOptions>& options,
+                                               bool initial_attempt) {
+    if (!initial_attempt) {
+        return dial(std::move(uri), options);


Suggested change

return dial(std::move(uri), options);

return dial(uri, options);

acmorrow

Just taking a look, random thoughts, feel free to do with them as you see fit.

acmorrow · 2025-03-03T16:12:18Z

src/viam/sdk/rpc/dial.hpp

    void set_credentials(boost::optional<Credentials> creds);
    void set_allow_insecure_downgrade(bool allow);
    void set_timeout(std::chrono::duration<float> timeout);
+    void set_initial_connection_attempts(int attempts);


A small proposal: these and the other set_x methods should return DialOptions& so you can chain them:

DialOptions options{}; options.set_entity(...).set_timeout(...)....;

acmorrow · 2025-03-03T16:16:14Z

src/viam/sdk/rpc/dial.hpp


+    // allows for specifying that this dial is an initial connection attempt, indicating that
+    // initial connection options should be used.
+    static std::shared_ptr<ViamChannel> dial(const char* uri,


I have something of a personal dislike for bool parameters like this, and it makes me wonder why this initial aspect isn't either just a setting on the DialOptions, or a differently named method.

My distaste arrises because at the call site, you end up like this:

ViamChannel::dial(my_uri, my_options, false);

What does false mean?

Whereas either of these makes the intention clear at the call site without needing to chase down the declaration / documentation for dial:

ViamChannel::dial(my_uri, my_options.as_initial());

or

ViamChannel::dial_initial(my_uri, my_options)

I like the look of the as_initial approach but I think it requires creating a copy of the options. So, went with dial_initial.

acmorrow · 2025-03-03T16:20:29Z

src/viam/sdk/rpc/dial.cpp

 }

+void DialOptions::set_initial_connection_attempt_timeout(std::chrono::duration<float> timeout) {
+    initial_connection_attempt_timeout_ = std::move(timeout);


I wonder if clang-tidy will think of it as a missed move? Personally, I always move things like these, since otherwise it feels like I'm making assumptions about implementation.

src/viam/sdk/rpc/dial.hpp

acmorrow · 2025-03-04T17:19:20Z

src/viam/sdk/rpc/dial.cpp

+        }
+    }
+
+    throw Exception(ErrorCondition::k_connection, "Unable to establish connection");


Should this maybe instead throw whatever the last exception was, so that the information about the cause of the failure isn't lost?

acmorrow · 2025-03-04T17:19:52Z

src/viam/sdk/rpc/dial.hpp

    static std::shared_ptr<ViamChannel> dial(const char* uri,
                                             const boost::optional<DialOptions>& options);

+    // Specifies that this dial is an initial connection attempt and that initial connection options


This seems as good a time as any to doc comment this function, as well as dial above.

Done; added a clause as well clarifying that use of dial is discouraged and that RobotClient::at_address(...) is preferred.

acmorrow · 2025-03-04T18:49:28Z

src/viam/sdk/rpc/dial.cpp

+            return connection;
+        } catch (const std::exception& e) {
+            attempts_remaining -= 1;
+            exception = e;


This is going to slice the exception, I think (i.e. only the std::exception part will be copied, but any subtype will be lost). I think you can just look at attempts_remaining inside the catch and throw if there are non left.

acmorrow

LGTM

initial connection options for cpp

4183152

stuqdog requested a review from a team as a code owner February 28, 2025 20:41

stuqdog requested review from lia-viam and njooma and removed request for a team February 28, 2025 20:41

lia-viam requested changes Mar 3, 2025

View reviewed changes

acmorrow reviewed Mar 3, 2025

View reviewed changes

pr comments, actually use new logic when creating robotclient

69eea20

stuqdog requested review from acmorrow and lia-viam March 3, 2025 21:17

lia-viam approved these changes Mar 3, 2025

View reviewed changes

acmorrow requested changes Mar 4, 2025

View reviewed changes

CR comments

caddc57

stuqdog requested a review from acmorrow March 4, 2025 18:40

acmorrow reviewed Mar 4, 2025

View reviewed changes

stuqdog added 2 commits March 4, 2025 14:01

just throw the exception

72c2f86

add throws to dial docstring

319cebd

stuqdog requested a review from acmorrow March 4, 2025 19:03

stuqdog added 2 commits March 4, 2025 14:42

add boost_unreachable

aff2e8a

Merge branch 'main' into RSDK-6158-initial-connection-options

e41ab0f

acmorrow approved these changes Mar 5, 2025

View reviewed changes

stuqdog merged commit 3d58df1 into viamrobotics:main Mar 5, 2025
4 checks passed

stuqdog deleted the RSDK-6158-initial-connection-options branch March 5, 2025 16:52

	return dial(std::move(uri), options);
	return dial(uri, options);

RSDK-6158 - initial connection options #376

RSDK-6158 - initial connection options #376

Uh oh!

Conversation

stuqdog commented Feb 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants