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
@@ -56,9 +61,11 @@ struct SpinnerMonitor
   */
   struct Entry
   {
-    Entry(const boost::thread::id &tid) : tid(tid), num(0) {}
+    Entry(const boost::thread::id &tid,
+          const boost::thread::id &initial_tid) : tid(tid), initial_tid(initial_tid), num(0) {}
 
     boost::thread::id tid; // proper thread id of single-threaded spinner
+    boost::thread::id initial_tid; // to retain old behaviour, store first spinner's thread id
     unsigned int num; // number of (alike) spinners serving this queue
   };
 
@@ -67,19 +74,35 @@ struct SpinnerMonitor
   {
     boost::mutex::scoped_lock lock(mutex_);
 
+    boost::thread::id current_tid = boost::this_thread::get_id();
     boost::thread::id tid; // current thread id for single-threaded spinners, zero for multi-threaded ones
     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() || // we will spin on any new queue
                       it->second.tid == tid ); // otherwise spinner must be alike (all multi-threaded: 0, or single-threaded on same thread id)
 
     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: single-threaded spinner after multi-threaded one(s)."
+                         << DEFAULT_ERROR_MESSAGE
+                         << " Only allowed for backwards compatibility.");
+        it->second.tid = tid; // "upgrade" tid to represent single-threaded spinner
+      }
+      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)));
 
     // increment number of active spinners
     it->second.num += 1;
@@ -112,9 +135,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
@@ -130,8 +150,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);
@@ -220,8 +240,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>