master::execute with wait_for_master=false is hiding comunication errors

[v-lopez]

Some methods such as get param in https://github.com/ros/ros_comm/blob/indigo-devel/clients/roscpp/src/libros/param.cpp#L312 are implemented with calls to master::execute with wait_for_master=false.

In this situation, a failure to get a parameter because it is not set cannot be distinguished from a failure due to connection errors with the master.

Some feedback with regards to the type of error should be provided, maybe by printing the error message in https://github.com/ros/ros_comm/blob/indigo-devel/clients/roscpp/src/libros/master.cpp#L206 even if wait_for_master is false.

[po1]

I don't know at all if this is feasible, but I'd favor a feedback in the logic rather than in the logs. I see three possible ways of doing this:

• the classical C approach with errno, which would be the least invasive but would mean keeping a global variable around, and ensuring that it is thread-local,
• changing the interface of validateXmlrpcResponse and master::execute so that we can get some feedback about what went wrong,
• and C++ exceptions, of which I am not really in favor here.

[dirk-thomas]

The problem with the proposed ways (2. and 3.) is that they would change the public interface which is not feasible to be applied in an already released ROS distro. And even for an upcoming distro it must be carefully done since that implies that everyone using the API needs to update its code - either to adapt it to the changed interface or catch the additional exception.

The first approach would be possible without changing the API. But currently ROS does not use that kind of error signaling which would make it kind of special.

I can't think of a different way right now which would not require an API change.

[po1]

The problem with the proposed ways (2. and 3.) is that they would change the public interface which is not feasible to be applied in an already released ROS distro. And even for an upcoming distro it must be carefully done since that implies that everyone using the API needs to update its code - either to adapt it to the changed interface or catch the additional exception.

The API can be extended without necessarily implying any breaks. Parts of it can be overloaded or have default arguments, so that old code should stay compatible. Before I spend too much time on this, would a patch that extends (but does not break) API be acceptable?

[dirk-thomas]

Sure.