CapabilityServer.__catch_and_log makes it hard to catch exceptions

[bit-pirate]

While improving the error handling in the rapp manager for issue #68, it took me a while to do the right exception handling. Only when I realised that CapabilityServer.__catch_and_log() redefines the RuntimeError exception raised by CapabilityServer.__free_capability(), I understood why my RuntimeError catching didn't work.

I'm under the assumption that doing a catch all via except Exception is bad practise. So, I'm wondering if __catch_and_log should be changed.

[wjwwood]

__catch_and_log is only used in the handling of service calls. These happen in threads, so putting a try-except around the main CapabilityServer thread will not catch them.

__catch_and_log re-raise's every exception it catches, it simply catches, logs, and then re-raise's. I don't understand why you think this is hampering your ability to catch errors?

[bit-pirate]

I'm using CapabilityClient.free_capability() and as you said, this is doing a service call to the capability server, where it get's handled by CapabilityServer.handle_free_capability(). This contains the catch_and_log. The original RuntimeError exception is actually raised two levels down at __free_capability().

From the error output I saw that there was a RuntimeError exception. So I tried to catch that, what didn't work. After I found the catch_and_log function, I realised, I had to catch a general Exception. That is what I was talking about.

However, I also must say I'm confused, that I receive this exception at all. From a ROS service call I'm expecting either a True or False return or in special cases a ROSException, e.g. the service is not available. Is there something wrong in this understanding?

[wjwwood]

You can take out the whole __catch_and_log function, but I think it will not change what you are seeing.

However, I also must say I'm confused, that I receive this exception at all. From a ROS service call I'm expecting either a True or False return or in special cases a ROSException, e.g. the service is not available. Is there something wrong in this understanding?

Services should raise a ServiceException from rospy.service across the network if the service call implementation raises. See my tests:

https://github.com/osrf/capabilities/blob/master/test/rostest/test_server/test_ros_services.py#L98-103

[bit-pirate]

Learning something new everyday! 😊

First, the re-raise in the __catch_and_log function actually preserves the exception type.
And second:

Services should raise a ServiceException from rospy.service across the network if the service call implementation raises. See my tests:

So, catching rospy.service.ServiceException did resolve this issue for me.

Now, I wonder though, if handling this exception should actually be done inside CapabilityClient.free_capability() and CapabilityClient.use_capability(), since from the outside it is not clear that this is calling a ROS service. What do you think?

[wjwwood]

Now, I wonder though, if handling this exception should actually be done inside CapabilityClient.free_capability() and CapabilityClient.use_capability(), since from the outside it is not clear that this is calling a ROS service. What do you think?

I think you may be right. I guess the question in my mind is what it should do instead. If a call to free_capability fails, should a different exception be raised or should an error code (bool) be returned?

It seems dangerous to rely on a return code value in Python, most people will not check that by default.