Catching any exception here is "safer", in that anything that happens to throw will be caught. But on the other hand, that same logic goes for every destructor in our codebase, and we don't wrap them all in try { } catch(...) {}. What I'm trying to get at here is that I think this may be better as:

} catch (const RCLErrorBase &) {

(looks deeper). Oh, but rclcpp::exception::throw_from_rcl_error can also throw a std::invalid_argument or std::runtime_error. That's unfortunate. Do you have any thoughts here on how we could avoid the catch-all, or do you think using the catch-all is the right solution?

I think it would be better to put try & catch for now, but i agree with @clalancette . although i do not have any better idea for now.

I wasn't terribly convinced of any one solution. The catch all is pretty useful, since reset is a function that can throw several different types of exceptions, and it's simple to write. reset gets plenty of usage in the unit tests, so I think if it were to start throwing a different type of exception we'd start seeing it show up there.

Other ideas I had...

rcl_lifecycle_state_fini currently only fails if it's passed a bad allocator, which would be a programming error. But if it did fail, it would only leak memory. I could move the RCLCPP_ERROR message to reset() and just ignore a bad return value. In that case, it might also be good to make reset() noexcept as well, so other exceptions thrown from inside that function are also identified and resolved.

I could remove the fault injection hook from rcl_lifecycle_state_fini. It wouldn't decrease coverage below 95% in rcl_lifecycle or rclcpp_lifecycle I believe.

I could catch a std::exception, which RclError is derived from, and log the e.what(). That would be about just as general as the ... though.

I could catch the RCLErrorBase, and let the std::exceptions from throw_from_rcl_error still crash since they also represent programming errors but won't be thrown during fault injection testing.

rcl_lifecycle_state_fini currently only fails if it's passed a bad allocator, which would be a programming error. But if it did fail, it would only leak memory. I could move the RCLCPP_ERROR message to reset() and just ignore a bad return value. In that case, it might also be good to make reset() noexcept as well, so other exceptions thrown from inside that function are also identified and resolved.

Oh, I like this option. It seems the cleanest to me. I don't think we'll run into any problems marking reset() as noexcept either; other than the throw_from_rcl_error, I don't think there is anything else in the method that can throw.

Switched to noexcept option