Migration guide recommends to throw exceptions in service callbacks

[afrixs]

Migration guide says that "Service callbacks in ROS 2 do not have boolean return values. Instead of returning false on failures, throwing exceptions is recommended."

However throwing an exception makes the node crash and client hangs on waiting for response. If I understand it well, @wjwwood speaks here (in the first paragraph) about two solutions:

1. To add a success field in the response message
2. To add a timeout on the client side and implement a SharedPtrDeferResponseCallback on the server side which doesn't send response in the case of failure, e.g.

// ROS 1 style is in comments, ROS 2 follows, uncommented.
// #include "nav_msgs/GetMap.h"
#include "nav_msgs/srv/get_map.hpp"

// bool service_callback(
//   nav_msgs::GetMap::Request & request,
//   nav_msgs::GetMap::Response & response)
void service_callback(
  const std::shared_ptr<rmw_request_id_t> request_header,
  const std::shared_ptr<nav_msgs::srv::GetMap::Request> request)
{
  auto response = std::make_shared<nav_msgs::srv::GetMap::Response>();
  // ...
  if (!has_map_) {
    // return false;
    return;
  }
  // return true;
  map_server_->send_response(*request_header, *response);
}

Note: Personally I think it would be great to have an implicit way to make the service fail inside the callback (what if an uneditable client lib is used without the 'success' field and what if it's undesirable to wait until timeout), however these workarounds are good enough for me for now so I'm just reporting the misleading recommendation in the docs.

[clalancette]

This is a good point, and I agree with your points. Would you mind opening up a pull request that updates the documentation with these recommendations? Thank you!