rosnode: Explicitly handle socket.timeout in rosnode ping #1517
Conversation
msadowski
changed the title
| @@ -336,6 +336,9 @@ def rosnode_ping(node_name, max_count=None, verbose=False): | |||
| if verbose: | |||
| print("xmlrpc reply from %s\ttime=%fms"%(node_api, dur)) | |||
| # 1s between pings | |||
| except socket.timeout: | |||
| print("connection to [%s] timed out"%node_name, file=sys.stderr) | |||
| return False | |||
There was a problem hiding this comment.
With this extra block added can line 364 still happen?
There was a problem hiding this comment.
Looking into documentation of socket I believe it should not happen when we handle timeout error explicitly.
Socket has 4 exceptions:
- socket.error - can return either a string or (errno, string)
- socket.herror - (h_errno, string)
- socket.gaierror - (error, string)
- socket.timeout - string
since the timeout is the only subclass of socket.error that returns a single string I think the line 364 should never happen (unless I'm missing anything else that could raise a ValueError that is not related to unpacking the error).
Would you like me to remove this ValueError check?
There was a problem hiding this comment.
since the timeout is the only subclass of socket.error that returns a single string I think the line 364 should never happen
The the following message should be changed to state that something "unknown" has happened, e.g.:
else:
print("connection to [%s] timed out"%node_name, file=sys.stderr)
replaced by:
else:
print("connection to [%s] failed"%node_name, file=sys.stderr)
Would you like me to remove this ValueError check?
I would rather keep that - there is no reason related to the issue here (the timeout) that it should be removed.
There was a problem hiding this comment.
Thanks for clarification. I just pushed the change.
|
Thanks for the patch. |
* Explicitly handle socket.timeout in rosnode ping * Change error string when handling socket.error
* Explicitly handle socket.timeout in rosnode ping * Change error string when handling socket.error
* Explicitly handle socket.timeout in rosnode ping * Change error string when handling socket.error
This pull request is in response to #1516. There are some cases where rosnode ping would take over 30 seconds to scan all the nodes due to ValueError happening when trying to unpack socket.error.
This pull request adds handling of socket.timeout explicitly.
Please let me know if there is anything else you would like me to change.