Catch the exception from rate.sleep() if the context is invalid.#2956
Catch the exception from rate.sleep() if the context is invalid.#2956fujitatomoya merged 1 commit intorollingfrom
Conversation
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an exception handling issue where Rate::sleep() would throw an exception when called on an invalid/shutdown ROS context. The change catches the exception and returns false instead, providing a more graceful failure mode.
Key changes:
- Added exception handling around
clock_->sleep_for()call inRate::sleep() - Added test coverage for the invalid context scenario
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rclcpp/src/rclcpp/rate.cpp | Wraps clock_->sleep_for() in try-catch to handle invalid context exceptions |
| rclcpp/test/rclcpp/test_rate.cpp | Adds test case to verify no exception is thrown when context is shutdown |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| rclcpp::Rate rate(1.0); | ||
| ASSERT_TRUE(rate.sleep()); | ||
| rclcpp::shutdown(); | ||
| EXPECT_NO_THROW(rate.sleep()); |
There was a problem hiding this comment.
this particular line is passing because of this PR fix. it did throw the exception without this PR change.
| clock_->sleep_for(time_to_sleep); | ||
| try { | ||
| // If the context is invalid, an exception will be thrown. | ||
| clock_->sleep_for(time_to_sleep); |
There was a problem hiding this comment.
It does not matter what kind of clock type is used by application.
During this sleep call, it checks if the context is valid in the underlying clock object. and this is required to register the shutdown callback to wake the condition variable to the clock.
|
@jmachowinski can you take a look at this? |
|
Ah, sorry i forgot to run the CI. i will revert and retry with CI. |
…2#2956) Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Description
Fixes #2862
Is this user-facing behavior change?
Yes.
before, it throws the exception via rate.sleep() call if the context is already shutdown by other threads.
after, it does not throw the exception via rate.sleep() call, instead it just returns the false to indicate that it could not sleep for the full time.
Did you use Generative AI?
No
Additional Information