Refactor TCPSocket::setup Timeout and Error Handling#511
Open
srvald wants to merge 6 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #511 +/- ##
==========================================
+ Coverage 77.14% 77.20% +0.06%
==========================================
Files 116 116
Lines 6354 6415 +61
Branches 2764 2792 +28
==========================================
+ Hits 4902 4953 +51
- Misses 1101 1106 +5
- Partials 351 356 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default mode and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 6618266. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Refactor TCPSocket::setup
Description
This PR refactors the
TCPSocket::setup()method to resolve the blocking behavior during network failures.The implementation now introduces an asynchronous approach to enforce a connection timeout and capture socket errors.
Step-by-Step Modifications
1. Signature Update (Timeout Parameter)
timeoutparameter (std::chrono::milliseconds) to thesetup()function. This makes it possible to define exactly how long to wait for a connection before giving up. Otherwise, the code blocks indefinitely, keeping the thread hanging even if the number of max tries is set to 1. The default value has been set to 500 ms.2. Error Tracking (
std::error_code)std::error_code socket_errorvariable.getLastSocketErrorCode()is called immediately after any failure to store the exact system reason (e.g.,ECONNREFUSED,EHOSTUNREACH) so it can be reported later.3. Non-Blocking Connection Logic
fcntl()andO_NONBLOCK.::connect()is called. If it returnsEINPROGRESS, the code now hands control over to::select().select()waits for the socket to become writable, strictly bounded by the newly providedtimeoutparameter.select()indicates readiness,::getsockopt(..., SO_ERROR, ...)is used to verify if the connection actually succeeded or if it failed in the background.4. Proper Resource Cleanup on Failure
socket_fd_is created but the connection attempt fails, it is now properly closed.::ur_close(socket_fd_)and reset toINVALID_SOCKETinside the failure branches within theforloop to properly release resources. Also added error capturing if::socket()creation fails directly.5. Improved Logging & Debugging
"Please check that the robot is booted and reachable...") with the exact system string translated bysocket_error.message()."Reason: Connection refused","Reason: Operation not permitted", or"Reason: Connection timed out", improving troubleshooting capabilities for end users.Testing Performed
select()and fails immediately.timeoutduration before logging a timeout error.Note
Medium Risk
Changes core TCP connection establishment logic (non-blocking connect +
select()/getsockopt()), which can affect connectivity behavior across platforms and edge cases like fd limits/timeouts.Overview
TCPSocket::setup()now accepts an additionaltimeout(default500ms) and uses a non-blocking connect withselect()to enforce connection timeouts instead of potentially blocking indefinitely.Connection attempts now capture and surface the underlying
std::error_codereason in retry logs, add extra failure-path cleanup (close/reset fd), and include a non-Windows guard forFD_SETSIZEoverflow before usingselect().Reviewed by Cursor Bugbot for commit 6b46158. Bugbot is set up for automated code reviews on this repo. Configure here.