{"action":"created","issue":{"url":"https://api.github.com/repos/ros/ros_comm/issues/928","repository_url":"https://api.github.com/repos/ros/ros_comm","labels_url":"https://api.github.com/repos/ros/ros_comm/issues/928/labels{/name}","comments_url":"https://api.github.com/repos/ros/ros_comm/issues/928/comments","events_url":"https://api.github.com/repos/ros/ros_comm/issues/928/events","html_url":"https://github.com/ros/ros_comm/pull/928","id":188331945,"number":928,"title":"Fix a bug that causes an XmlRpc::XmlRpcException","user":{"login":"afakihcpr","id":13023458,"avatar_url":"https://avatars.githubusercontent.com/u/13023458?v=3","gravatar_id":"","url":"https://api.github.com/users/afakihcpr","html_url":"https://github.com/afakihcpr","followers_url":"https://api.github.com/users/afakihcpr/followers","following_url":"https://api.github.com/users/afakihcpr/following{/other_user}","gists_url":"https://api.github.com/users/afakihcpr/gists{/gist_id}","starred_url":"https://api.github.com/users/afakihcpr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/afakihcpr/subscriptions","organizations_url":"https://api.github.com/users/afakihcpr/orgs","repos_url":"https://api.github.com/users/afakihcpr/repos","events_url":"https://api.github.com/users/afakihcpr/events{/privacy}","received_events_url":"https://api.github.com/users/afakihcpr/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-11-09T19:29:23Z","updated_at":"2016-11-22T15:50:34Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ros/ros_comm/pulls/928","html_url":"https://github.com/ros/ros_comm/pull/928","diff_url":"https://github.com/ros/ros_comm/pull/928.diff","patch_url":"https://github.com/ros/ros_comm/pull/928.patch"},"body":"The exception is raised  if the node is trying to subscribe to a topic after rosmaster dies. In this case,  the node will be stuck in an infinite loop in `ros::master::execute()`:\r\n\r\nhttps://github.com/ros/ros_comm/blob/kinetic-devel/clients/roscpp/src/libros/master.cpp#L189\r\n\r\nWhen the node is sent a sigint, it will exit this loop and `execute()` will return with an invalid `payload` that causes the exception when trying to do `payload.size()`.\r\n\r\nA simple example to reproduce:\r\n\r\n```\r\n#include \"ros/ros.h\"\r\n#include \"std_msgs/String.h\"\r\n#include <ros/topic.h>\r\n\r\nvoid callback(const std_msgs::String::ConstPtr& msg)\r\n{\r\n  ROS_ERROR(msg->data.c_str());\r\n}\r\n\r\nint main(int argc, char **argv)\r\n{\r\n  ros::init(argc, argv, \"test_node\");\r\n  ros::NodeHandle n;\r\n\r\n  boost::shared_ptr<std_msgs::String const> msg;\r\n  while (!(msg = ros::topic::waitForMessage<std_msgs::String>(\"some_topic\", n, ros::Duration(2))) && ros::ok())\r\n  {\r\n    ROS_ERROR(\"Waiting for messages\");\r\n  }\r\n  \r\n  ros::spin();\r\n  return 0;\r\n}\r\n```\r\n\r\nIf we run roscore, then run the above node, then kill roscore then try to stop the node with Ctrl-C we will get:\r\n` terminate called after throwing an instance of ' XmlRpc::XmlRpcException'`\r\n\r\nAn alternative solution would be to add at the end of `ros::master::execute()` before `return true`:\r\n```\r\nif (!ros::ok())\r\n{\r\n  return false;\r\n}\r\nreturn true;\r\n```\r\n\r\n@dirk-thomas \r\nFYI @mikepurvis  @efernandez "},"comment":{"url":"https://api.github.com/repos/ros/ros_comm/issues/comments/262278030","html_url":"https://github.com/ros/ros_comm/pull/928#issuecomment-262278030","issue_url":"https://api.github.com/repos/ros/ros_comm/issues/928","id":262278030,"user":{"login":"mikepurvis","id":944666,"avatar_url":"https://avatars.githubusercontent.com/u/944666?v=3","gravatar_id":"","url":"https://api.github.com/users/mikepurvis","html_url":"https://github.com/mikepurvis","followers_url":"https://api.github.com/users/mikepurvis/followers","following_url":"https://api.github.com/users/mikepurvis/following{/other_user}","gists_url":"https://api.github.com/users/mikepurvis/gists{/gist_id}","starred_url":"https://api.github.com/users/mikepurvis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mikepurvis/subscriptions","organizations_url":"https://api.github.com/users/mikepurvis/orgs","repos_url":"https://api.github.com/users/mikepurvis/repos","events_url":"https://api.github.com/users/mikepurvis/events{/privacy}","received_events_url":"https://api.github.com/users/mikepurvis/received_events","type":"User","site_admin":false},"created_at":"2016-11-22T15:50:34Z","updated_at":"2016-11-22T15:50:34Z","body":"Some related discussion in #836. Looks like @afakihcpr found the magic code path where `execute` returns true, but the payload is invalid— it occurs when the loop exits due to the ROS master shutting down: https://github.com/ros/ros_comm/blob/674263142a0be1124b273bdb1e0a79c3c956e259/clients/roscpp/src/libros/master.cpp#L238\r\n\r\nMy preference would be to handle this in `execute`, by changing the highlighted line to instead be:\r\n\r\n```\r\nif (ros::isShuttingDown() || XMLRPCManager::instance()->isShuttingDown())\r\n{\r\n  return false;\r\n}\r\n```\r\n\r\nI don't see this as a super-disruptive change; it is correcting a wrong behaviour in that execute should never return true unless the RPC succeeded and `payload` is valid."}}
{"actor":{"display_login":"mikepurvis"}}