Avoid deleting XmlRpcClient's while they are being still in use on another thread

[afakihcpr]

@dirk-thomas @mikepurvis @efernandez @jasonimercer

Bug description:

Deleting XmlRpcClient's in XMLRPCManager::shutdown() could lead to crashes or to the process getting blocked if a thread tries to communicate with the master while the clients are being deleted. The issue is in the following code at line 158 of xmlrpc_manager.cpp:

// kill the last few clients that were started in the shutdown process
  for (V_CachedXmlRpcClient::iterator i = clients_.begin();
       i != clients_.end(); ++i)
  {
    for (int wait_count = 0; i->in_use_ && wait_count < 10; wait_count++)
    {
      ROSCPP_LOG_DEBUG("waiting for xmlrpc connection to finish...");
      ros::WallDuration(0.01).sleep();
    }

    i->client_->close();
    delete i->client_;
  }

There are three potential crashes that could result from this:

1. The clients_ vector is not protected while deleting. A thread can attempt to get a client while the loop is running and it could get a client that has been deleted. This would cause a crash somewhere in
   XmlRpcClient::execute or could cause the node to be blocked at select.
2. Also, if many clients are being requested repeatedly during the time when shutdown() is sleeping and waiting for a client to become out of use, and being served with that same client, the wait could timeout with the client still in use, and it would get deleted while being in use.
3. Finally, there's a chance, that getXMLRpcClient would push_back a new client into clients triggering a re-allocation and invalidating clients which would cause a crash in the delete loop itself.

Code to reproduce:

#include "ros/ros.h"
#include <boost/thread.hpp>
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/thread/thread.hpp>

void run()
{
  ros::NodeHandle nh("~");
  std::string s = "something";
  const int iterations = 100;
  while (true)
  {
    for (int i = 0; i < iterations; ++i)
    {
      nh.param("param", s, s);
    } 
    boost::this_thread::interruption_point();
  }
}

int main(int argc, char **argv)
{
  ros::init(argc, argv, "test_node");

  boost::thread t(run);
  boost::this_thread::sleep(boost::posix_time::milliseconds(1000));
    
  ros::shutdown();

  t.interrupt();
  t.join();

  return 0;
}

Resolution

• Acquire clients_mutex_ before deleting the clients
• Remove the timed wait for the clients to become not in use
• Only delete and erase clients that are not in use
• Clients that would still be in use would delete themselves on release

[efernandez]

👍

LGTM and since this also have some testing, I wouldn't do bigger changes.

I wonder how we could adapt the code that reproduces the problem into a unit test, and where it should live. ❓

[dirk-thomas]

With the current patch the shutdown method doesn't sleep (and therefore yield) anymore if clients are still in use. Isn't that something which would make sense to retain?

[afakihcpr]

@dirk-thomas If it is desirable to yield in shutdown for clients that are still in use, we could add after removing the clients Not in use:

for (int wait_count = 0; !clients_.empty() && wait_count < 10; wait_count++)
{	
  ROSCPP_LOG_DEBUG("waiting for xmlrpc connection to finish...");
  ros::WallDuration(0.01).sleep();
}

This will give time to the clients in use to finish and remove themselves from clients_. Does that sound like an acceptable solution?

[dirk-thomas]

Yes, I think that would be good to closer match the original behavior.

[mikepurvis]

We've been running this for a month; I think it's a safe merge.

[dirk-thomas]

@afakihcpr Thank you for the patch and for iterating on it. @mikepurvis Thanks for testing it.