Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persistent services frozen if service call returns false #168

Closed
v-lopez opened this issue Feb 7, 2013 · 4 comments
Closed

Persistent services frozen if service call returns false #168

v-lopez opened this issue Feb 7, 2013 · 4 comments
Assignees

Comments

@v-lopez
Copy link
Contributor

v-lopez commented Feb 7, 2013

Hi, I'm having the following issue.

Using a persistent service client and calling a service that will returns false blocks the persistent client in the call method.

I'm using ROS Fuerte, installed from this debian: ros-fuerte-ros-comm_1.8.15-0precise-20120908-1755-+0000_i386.deb

This issue is critical because of what I described here: #149 (comment)

We're forced to use persistent services, because non-persistent calls create and destroy so many sockets that it slow downs our computer and affects heavily the global performance. But we're having many show-stopping issues using them such as the one described in this ticket and the one in #149

I have a test that reproduces this issue but I can't upload it here, as it seems that it only accepts images.

@v-lopez
Copy link
Contributor Author

v-lopez commented Feb 7, 2013

Tested on the latest groovy version in the debian packages and still happens.

If you compile and execute the server below, you'll see that on the 10th iteration the server calls and never finishes the call.

Service server:

#include "ros/ros.h"
#include "beginner_tutorials/AddTwoInts.h"

bool add(beginner_tutorials::AddTwoInts::Request  &req,
         beginner_tutorials::AddTwoInts::Response &res)
{
  res.sum = req.a + req.b;
  ROS_INFO("request: x=%ld, y=%ld", (long int)req.a, (long int)req.b);
  ROS_INFO("sending back response: [%ld]", (long int)res.sum);

  //If this number is provided, the service will return false
  if (req.b == 1234)
     return false;
  return true;
}

int main(int argc, char **argv)
{
  ros::init(argc, argv, "add_two_ints_server");
  ros::NodeHandle n;

  ros::ServiceServer service = n.advertiseService("add_two_ints", add);
  ROS_INFO("Ready to add two ints.");
  ros::spin();

  return 0;
}

Service client:

#include "ros/ros.h"
#include "beginner_tutorials/AddTwoInts.h"


int main(int argc, char **argv)
{
  ros::init(argc, argv, "persistent_service_test");
  ros::NodeHandle n;

  ros::ServiceClient client = n.serviceClient<beginner_tutorials::AddTwoInts>("add_two_ints", true);

  beginner_tutorials::AddTwoInts srv;
  srv.request.a = 2;
  for (int i = 0; i < 100; i++)
  {
    srv.request.b = 2;

    if (i == 10)   //On iteration number 10, we send a number that is hard coded to return false on the server 
      srv.request.b = 1234;
   else
      srv.request.b = 2;

   ROS_INFO("Calling service server, iteration %d", i);
   if (client.call(srv))
   {
    ROS_INFO("Call successfull");
   }
   else
   {
     ROS_ERROR("Failed to call service ");
  }
}

  return 0;
}

@dpinol
Copy link

dpinol commented Feb 14, 2013

I found the problem. It's actually at roscpp_core/roscpp_serialization/include/ros/serialization.h
This commit ros/roscpp_core@a5b8d2c mad the failing service calls to always return the exception, but forgot to serialize the message size before the message.
For me it works if I just revert the code to send always "ok" and message in the same way no matter if the call failed or succeeded.

/**

  • \brief Serialize a service response
    */
    template
    inline SerializedMessage serializeServiceResponse(bool ok, const M& message)
    {
    SerializedMessage m;

// if (ok)
{
uint32_t len = serializationLength(message);
m.num_bytes = len + 5;
m.buf.reset(new uint8_t[m.num_bytes]);

OStream s(m.buf.get(), (uint32_t)m.num_bytes);
serialize(s, (uint8_t)ok);
serialize(s, (uint32_t)m.num_bytes - 5);
serialize(s, message);

}
/* else
{
uint32_t len = serializationLength(message);
m.num_bytes = len + 1;
m.buf.reset(new uint8_t[m.num_bytes]);

OStream s(m.buf.get(), (uint32_t)m.num_bytes);
serialize(s, (uint8_t)ok);
serialize(s, message);

}
*/
return m;
}

@ghost ghost assigned dirk-thomas Feb 14, 2013
@dirk-thomas
Copy link
Member

Reverting the above mentioned change breaks other case i.e. passing an exception to the client.

@dpinol
Copy link

dpinol commented Mar 8, 2013

Thank you Dirk, it now works perfectly!

PierrickKoch referenced this issue in PierrickKoch/robotpkg Feb 21, 2014
Changes since 1.8.15:

1.9.46 (2013-06-18)
-------------------

* add dependency on roslisp
  (`#240 <https://github.com/ros/ros_comm/issues/240>`_)
* fix crash in bag migration
  (`#239 <https://github.com/ros/ros_comm/issues/239>`_)
* add CMake function roslaunch_add_file_check()
  (`#241 <https://github.com/ros/ros_comm/issues/241>`_)
* fix rosnode_ping to check if new node uri is valid before using it
  (`#235 <https://github.com/ros/ros_comm/issues/235>`_)

1.9.45 (2013-06-06)
-------------------
* improve handling of UDP transport, when fragmented packets are lost or arive
  out-of-order the connection is not dropped anymore, onle a single message is
  lost (`#226 <https://github.com/ros/ros_comm/issues/226>`_)
* fix missing generation of constant definitions for services
  (`ros/gencpp#2 <https://github.com/ros/gencpp/issues/2>`_)
* fix restoring thread context when callback throws an exception
  (`#219 <https://github.com/ros/ros_comm/issues/219>`_)
* fix calling PollManager::shutdown() repeatedly
  (`#217 <https://github.com/ros/ros_comm/issues/217>`_)
* add missing run_depend on python-yaml
* allow configuration of ports for XML RPCs and TCP ROS
* fix race condition where rospy subscribers do not connect to all publisher
* fix closing and deregistering connection when connect fails
  (`#128 <https://github.com/ros/ros_comm/issues/128>`_)
* fix log level of RosOutHandler
  (`#210 <https://github.com/ros/ros_comm/issues/210>`_)
* added option '--duration' to 'rosbag play'
  (`#121 <https://github.com/ros/ros_comm/issues/121>`_)
* fix missing newlines in rosbag error messages
  (`#237 <https://github.com/ros/ros_comm/issues/237>`_)
* fix flushing for tools like 'rosbag compress'
  (`#237 <https://github.com/ros/ros_comm/issues/237>`_)
* add warnings for obviously wrong environment variables ROS_HOSTNAME and
  ROS_IP (`#134 <https://github.com/ros/ros_comm/issues/134>`_)
* fix exception on netifaces.ifaddresses()
  (`#211 <https://github.com/ros/ros_comm/issues/211>`_,
  `#213 <https://github.com/ros/ros_comm/issues/213>`_)
  (regression from 1.9.42)
* modify rosnode_ping to check for changed node uri if connection is refused
  (`#221 <https://github.com/ros/ros_comm/issues/221>`_)
* allow passing arguments to add_rostest(ARGS ...)
 (`#232 <https://github.com/ros/ros_comm/issues/232>`_)
* modified roslaunch $(find PKG) to consider path behind it for resolve
  strategy (`#233 <https://github.com/ros/ros_comm/pull/233>`_)
* add boolean attribute 'subst_value' to rosparam tag in launch files
  (`#218 <https://github.com/ros/ros_comm/issues/218>`_)
* add command line parameter to print out launch args
* fix missing import in arg_dump.py
* fix template syntax for signal_.template addCallback() to work with Intel
  compiler

1.9.44 (2013-03-21)
-------------------
* fix install destination for dll's under Windows
* fix various issues on Windows
  (`#189 <https://github.com/ros/ros_comm/issues/189>`_)
* fix 'roslaunch --files' with non-unique anononymous ids
  (`#186 <https://github.com/ros/ros_comm/issues/186>`_)
* fix ROS_MASTER_URI for Windows

1.9.43 (2013-03-13)
-------------------
* implement process killer for Windows
* fix exports of message filter symbols for Windows

1.9.42 (2013-03-08)
-------------------
* improve speed of message generation in dry packages
  (`#183 <https://github.com/ros/ros_comm/issues/183>`_)
* fix roscpp service call deadlock
  (`#149 <https://github.com/ros/ros_comm/issues/149>`_)
* fix freezing service calls when returning false
  (`#168 <https://github.com/ros/ros_comm/issues/168>`_)
* fix error message publishing wrong message type
  (`#178 <https://github.com/ros/ros_comm/issues/178>`_)
* fix missing explicit dependency on pthread
  (`#135 <https://github.com/ros/ros_comm/issues/135>`_)
* fix compiler warning about wrong comparison of message md5 hashes
  (`#165 <https://github.com/ros/ros_comm/issues/165>`_)
* make dependencies on rospy optional by refactoring RosStreamHandler to
  rosgraph (`#179 <https://github.com/ros/ros_comm/issues/179>`_)
* added option '--duration' to 'rosrun rosbag play'
  (`#121 <https://github.com/ros/ros_comm/issues/121>`_)
* add error message to rosbag when using same in and out file
  (`#171 <https://github.com/ros/ros_comm/issues/171>`_)
* fix handling spaces in folder names
  (`ros/catkin#375 <https://github.com/ros/catkin/issues/375>`_)
* replace custom code with Python module netifaces
  (`#130 <https://github.com/ros/ros_comm/issues/130>`_)
* make dependencies on rospy optional by refactoring RosStreamHandler to
  rosgraph (`#179 <https://github.com/ros/ros_comm/issues/179>`_)
* add option --skip-log-check
  (`#133 <https://github.com/ros/ros_comm/issues/133>`_)
* update API doc to list raised exceptions in config.py
* fix invocation of Python scripts under Windows
  (`#54 <https://github.com/ros/ros_comm/issues/54>`_)
* fix usage of rosservice from within a launch file
* fix missing run_depend on rosbag
  (`#179 <https://github.com/ros/ros_comm/issues/179>`_)

1.9.41 (2013-01-24)
-------------------
* allow sending data exceeding 2GB in chunks
  (`#4049 <https://code.ros.org/trac/ros/ticket/4049>`_)
* update getParam() doc
  (`#1460 <https://code.ros.org/trac/ros/ticket/1460>`_)
* add param::get(float)
  (`#3754 <https://code.ros.org/trac/ros/ticket/3754>`_)
* update inactive assert when publishing message with md5sum *, update related
  tests (`#3714 <https://code.ros.org/trac/ros/ticket/3714>`_)
* fix ros master retry timeout
  (`#4024 <https://code.ros.org/trac/ros/ticket/4024>`_)
* fix inactive assert when publishing message with wrong type
  (`#3714 <https://code.ros.org/trac/ros/ticket/3714>`_)
* improve performance of $(find ...)

1.9.40 (2013-01-13)
-------------------
* add colorization for rospy log output
  (`#3691 <https://code.ros.org/trac/ros/ticket/3691>`_)
* fix socket polling under Windows
  (`#3959 <https://code.ros.org/trac/ros/ticket/3959>`_)
* fix bagsort script (`#42 <https://github.com/ros/ros_comm/issues/42>`_)
* fix dependent packages by pass LOG4CXX include dirs and libraries along
* fix usage of variable arguments in vFormatToBuffer() function
* add colorization for rospy log output
  (`#3691 <https://code.ros.org/trac/ros/ticket/3691>`_)
* fix 'roslaunch --pid=' when pointing to ROS_HOME but folder does not exist
  (`#43 <https://github.com/ros/ros_comm/issues/43>`_)
* fix 'roslaunch --pid=' to use shell expansion for the pid value
  (`#44 <https://github.com/ros/ros_comm/issues/44>`_)
* fix output of 'rossrv --help'
  (`#3979 <https://code.ros.org/trac/ros/ticket/3979>`_)
* add support for boolean in 'rostopic -p'
  (`#3948 <https://code.ros.org/trac/ros/ticket/3948>`_)
* add checks for pip packages and rosdep
* fix check for catkin_pkg
* fix for thread race condition causes incorrect graph connectivity analysis
contradict pushed a commit to contradict/ros_comm that referenced this issue Aug 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants