rollback strict error checking for backwards compatibility

Previously, we could have the following situations:

1. `S1` (single-threaded spinner started in `thread 1`): will block `thread 1` until shutdown. Any further spinners in different threads were not allowed (with an error message).
2. `M1` (multi-threaded spinner started in `thread 1`): Further spinners started from _different_ threads were not allowed (with an error message).
3. `M1 ... M1 S1` (multi-threaded spinners started in `thread 1` and afterwards a single-threaded one started): This was accepted without any errors. But the new behavior is to reject `S1`!

Restrictions of case 1 + 2 are relaxed with this PR: Other spinners are allowed as long as the operate on a different queue. Thread doesn't matter.

The tricky part is case 3, which - although nonsense - was perfectly valid code before.
In order to maintain the old behavior, I need to remember, which thread the first M-spinner was started in, using the new variable `initial_tid`.

* allow spinning of a single-threaded spinner after some multi-threaded ones, as long as they are started from the same thread
* don't throw exceptions
* disabled corresponding unittests

clients/roscpp/src/libros/spinner.cpp

@@ -34,6 +34,11 @@
 
 namespace {
 
+const std::string DEFAULT_ERROR_MESSAGE =
+    "\nAttempt to spin a callback queue from two spinners, one of them being single-threaded."
+    "\nThis will probably result in callbacks being executed out-of-order."
+    "\nIn future this will throw an exception!";
+
 /** class to monitor running single-threaded spinners.
  *
  *  Calling the callbacks of a callback queue _in order_, requires a unique SingleThreadedSpinner
@@ -55,9 +60,11 @@ struct SpinnerMonitor
   */
   struct Entry
   {
-    Entry(const boost::thread::id &tid) : tid(tid), num_multi_threaded(0) {}
+    Entry(const boost::thread::id &tid,
+          const boost::thread::id &initial_tid) : tid(tid), initial_tid(initial_tid), num_multi_threaded(0) {}
 
     boost::thread::id tid; // thread id of single-threaded spinner
+    boost::thread::id initial_tid; // to retain old, but wrong behaviour
     unsigned int num_multi_threaded; // number of multi-threaded spinners
   };
 
@@ -66,17 +73,29 @@ struct SpinnerMonitor
   {
     boost::mutex::scoped_lock lock(mutex_);
 
+    boost::thread::id current_tid = boost::this_thread::get_id();
     boost::thread::id tid;
     if (single_threaded)
-      tid = boost::this_thread::get_id();
+      tid = current_tid;
 
     std::map<ros::CallbackQueue*, Entry>::iterator it = spinning_queues_.find(queue);
     bool can_spin = ( it == spinning_queues_.end() || it->second.tid == tid );
+
     if (!can_spin)
-      return false;
+    {
+      // Previous behavior (up to Kinetic) was to accept multiple spinners on a queue
+      // as long as they were started from the same thread. Although this is wrong behavior,
+      // we retain it here for backwards compatibility, i.e. we allow spinning of a
+      // single-threaded spinner after several multi-threaded ones, given that they
+      // were started from the same initial thread
+      if (it->second.initial_tid == tid)
+        ROS_ERROR_STREAM("SpinnerMonitor: " << DEFAULT_ERROR_MESSAGE);
+      else
+        return false;
+    }
 
     if (it == spinning_queues_.end())
-      it = spinning_queues_.insert(it, std::make_pair(queue, Entry(tid)));
+      it = spinning_queues_.insert(it, std::make_pair(queue, Entry(tid, current_tid)));
 
     if (!single_threaded) // increment number of multi-threaded spinners
       it->second.num_multi_threaded += 1;
@@ -114,9 +133,6 @@ struct SpinnerMonitor
 };
 
 SpinnerMonitor spinner_monitor;
-const std::string DEFAULT_ERROR_MESSAGE =
-    "Attempt to spin a callback queue from two spinners, one of them being single-threaded. "
-    "This will probably result in callbacks being executed out-of-order.";
 }
 
 namespace ros
@@ -132,8 +148,8 @@ void SingleThreadedSpinner::spin(CallbackQueue* queue)
 
   if (!spinner_monitor.add(queue, true))
   {
-    ROS_FATAL_STREAM("SingleThreadedSpinner: " << DEFAULT_ERROR_MESSAGE);
-    throw std::runtime_error("There is already another spinner on this queue");
+    ROS_ERROR_STREAM("SingleThreadedSpinner: " << DEFAULT_ERROR_MESSAGE);
+    return;
   }
 
   ros::WallDuration timeout(0.1f);
@@ -222,8 +238,8 @@ void AsyncSpinnerImpl::start()
 
   if (!spinner_monitor.add(callback_queue_, false))
   {
-    ROS_FATAL_STREAM("AsyncSpinnerImpl: " << DEFAULT_ERROR_MESSAGE);
-    throw std::runtime_error("There is already a single-threaded spinner on this queue");
+    ROS_ERROR_STREAM("AsyncSpinnerImpl: " << DEFAULT_ERROR_MESSAGE);
+    return;
   }
 
   continue_ = true;

test/test_roscpp/test/launch/spinners.xml

@@ -1,8 +1,4 @@
 <launch>
   <test pkg="test_roscpp" type="test_roscpp-spinners" test-name="Spinners_spin" args="--gtest_filter=Spinners.spin"/>
-  <test pkg="test_roscpp" type="test_roscpp-spinners" test-name="Spinners_spinfail" args="--gtest_filter=Spinners.spinfail"/>
-  <test pkg="test_roscpp" type="test_roscpp-spinners" test-name="Spinners_singlefail" args="--gtest_filter=Spinners.singlefail"/>
   <test pkg="test_roscpp" type="test_roscpp-spinners" test-name="Spinners_multi" args="--gtest_filter=Spinners.multi"/>
-  <test pkg="test_roscpp" type="test_roscpp-spinners" test-name="Spinners_multifail" args="--gtest_filter=Spinners.multifail"/>
-  <test pkg="test_roscpp" type="test_roscpp-spinners" test-name="Spinners_async" args="--gtest_filter=Spinners.async"/>
 </launch>