-
Notifications
You must be signed in to change notification settings - Fork 311
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.
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
wait for service in client example #93
Conversation
Since the problem probably affects all of the service tests does it make sense to only patch it for this single one? |
@dirk-thomas The right fix is to implement the |
// return true, hoping for the best. | ||
rmw_reset_error(); | ||
success = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fairly blanket error case, maybe you could either check the error message contents or at least print it out so that when there is an error that doesn't match what you expected you can see it in the log? That might break your example test though.
+1 The change makes sense to me. It's unfortunate that FastRTPS doesn't implement the solution I suggested to you 😢. |
This change made more tests fail on Linux and it didn't even build on Windows. Fun! |
7156b7f
to
fafad35
Compare
fafad35
to
104b5e7
Compare
Windows build fixed and same logic added to async client (it'll be removed from both examples once we resolve ros2/rclcpp#201). New CI: |
CI shows only failures and warning that are unrelated to this change. |
@wjwwood You +1ed previously, but can you have a second look before I merge? |
success = true; | ||
break; | ||
} | ||
} catch (std::runtime_error & e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue here is that node->count_subscribers
should never throw a C++ exception. This is actually a problem with the fast rtps implementation, but ideally it would catch the exception internally and convert it to a return code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have said, it should never throw a C++ exception because it is a C function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying function rmw_count_subscribers()
is C (for fastrtps, it's hardcoded to return an error: https://github.com/eProsima/ROS-RMW-Fast-RTPS-cpp/blob/master/rmw_fastrtps_cpp/src/functions.cpp#L1793-L1801), but here I'm calling a C++ class method: https://github.com/ros2/rclcpp/blob/master/rclcpp/src/rclcpp/node.cpp#L317-L329.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad. Sorry for the noise 🔔.
When the logic is fixed to actually require that |
The test that uses this client example can fail if the server isn't yet up and ready to hear the client's request. In that situation, the client waits forever and the test times out, as we've seen with FastRTPS. This PR adds a
wait_for_service()
call that the client uses to delay its request until the time is right.This function should not live here in the long term, but rather be improved and promoted into client API (ros2/rclcpp#201). But this is a decent place to park it for now.