Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rosnode: Explicitly handle socket.timeout in rosnode ping #1517

Merged
merged 2 commits into from Oct 8, 2018
Merged

rosnode: Explicitly handle socket.timeout in rosnode ping #1517

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d7a2d6
Explicitly handle socket.timeout in rosnode ping
msadowski Oct 4, 2018
4c59f50
Change error string when handling socket.error
msadowski Oct 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions tools/rosnode/src/rosnode/__init__.py
View file
Open in desktop
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this extra block added can line 364 still happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification. I just pushed the change.

except socket.error as e:
# 3786: catch ValueError on unpack as socket.error is not always a tuple
try:
Expand Down