Add timeout fundamentals for daemon client communication #886
Conversation
This file contains 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
Signed-off-by: Scott K Logan <[email protected]>
cottsay
force-pushed
the
cottsay/daemon-client-timeout
branch
from
22bd616 to
1a0f95e
Compare
fujitatomoya
reviewed
Mar 6, 2024
Comment on lines
+76
to
+77
| for the daemon node to respond. If it is not given, | ||
| the global default timeout setting is used. |
There was a problem hiding this comment.
using global default timeout would never time out with my environment... trying to access the server to list the method in localhost, it is not expected to take more than 10 seconds, maybe we can set specific timeout in default here?
There was a problem hiding this comment.
I can't think of any reasonable circumstances where it would take longer than that either. I'm in favor of a default timeout as well, but I think we need more feedback on what value might be appropriate.
I also think we need to improve the error message if it becomes the default behavior. Right now, the exception is unhandled.
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.
This PR is more of a proof-of-concept than a concrete proposal.
If there is a broken ROS 2 daemon process or another completely unrelated TCP server listening on the corresponding XMLRPC port, it's possible for calls like
is_daemon_runningto hang for a VERY long time.For example:
nc -k -l 11511ros2 daemon statusYou can see the XMLRPC request on the server, but without a response, the call to
ros2 daemon statuswill just sit there. I'm not sure how long it will go before the "global default" timeout will kick in, but I haven't waited long enough to see it.This can be a particularly bad problem in this package's tests, many of which connect to and sometimes create and destroy daemon processes. It would be nice if those tests didn't hang.
Possible mitigation for problems like #610 and #737