roscpp: fix potential busy-wait loop caused by backported Boost condi…

…tion_variable (fix ros#1343) ros#1014 and ros#1250 introduced a backported version of boost::condition_variable, where support for steady (monotonic) clocks has been added in version 1.61. But the namespace of the backported version was not changed and the symbol names might clash with the original version. Because the underlying clock used for the condition_variable is set in the constructor and must be consistent with the the expectations within member variables. The compiler might choose to inline one or the other or both, and is more likely to do so for optimized Release builds. But if it does not, the symbol ends up in the symbol table of roscpp and depending on which other libraries will be linked into the process it is unpredictable which of the two versions will be actually called at the end. In case the constructor defined in `/usr/include/boost/thread/pthread/condition_variable.hpp` was called and did not configure the internal pthread condition variable for monotonic clock, each call to the backported do_wait_until() method with a monotonic timestamp will return immediately and hence causes `CallbackQueue::callOne(timeout)` or `CallbackQueue::callAvailable(timeout)` to return immediately. This patch changes the namespace of the backported condition_variable implementation to boost_161. This removes the ambiguity with the original definition if both are used in the same process.
meyerj · Mar 12, 2019 · a3b370e · a3b370e
1 parent b0dcc75
commit a3b370e
Show file tree

Hide file tree

Showing 12 changed files with 69 additions and 146 deletions.
diff --git a/clients/roscpp/CMakeLists.txt b/clients/roscpp/CMakeLists.txt
@@ -20,6 +20,18 @@ list(GET roscpp_VERSION_LIST 0 roscpp_VERSION_MAJOR)
 list(GET roscpp_VERSION_LIST 1 roscpp_VERSION_MINOR)
 list(GET roscpp_VERSION_LIST 2 roscpp_VERSION_PATCH)
 
+# Make sure we use CLOCK_MONOTONIC for the condition variable wait_for if not Apple.
+if(NOT APPLE)
+  if(Boost_VERSION LESS 106100)
+    set(ROSCPP_USE_BACKPORTED_BOOST_CONDITION_VARIABLE_IMPLEMENTATION ON)
+  endif()
+  if(Boost_VERSION LESS 106700)
+    # CLOCK_MONOTONIC became the default in Boost 1.67:
+    # https://github.com/boostorg/thread/commit/1e84b978b2bb0aae830cc14533dea3b7ddda5cde
+    add_definitions(-DBOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC)
+  endif()
+endif()
+
 configure_file(${CMAKE_CURRENT_SOURCE_DIR}/include/ros/common.h.in ${CATKIN_DEVEL_PREFIX}/${CATKIN_GLOBAL_INCLUDE_DESTINATION}/ros/common.h)
 
 find_package(Boost REQUIRED COMPONENTS chrono filesystem system)

diff --git a/clients/roscpp/include/boost_161_condition_variable.h b/clients/roscpp/include/boost_161_condition_variable.h
@@ -1,5 +1,5 @@
-#ifndef BOOST_THREAD_CONDITION_VARIABLE_HPP
-#define BOOST_THREAD_CONDITION_VARIABLE_HPP
+#ifndef BOOST_161_THREAD_CONDITION_VARIABLE_HPP
+#define BOOST_161_THREAD_CONDITION_VARIABLE_HPP
 
 //  condition_variable.hpp
 //
@@ -12,6 +12,9 @@
 #include <boost/thread/detail/platform.hpp>
 #if defined(BOOST_THREAD_PLATFORM_WIN32)
 #include <boost/thread/win32/condition_variable.hpp>
+namespace boost_161 {
+  using condition_variable = boost::condition_variable;
+}
 #elif defined(BOOST_THREAD_PLATFORM_PTHREAD)
 //#include <boost/thread/pthread/condition_variable.hpp>
 #include "boost_161_pthread_condition_variable.h"

diff --git a/clients/roscpp/include/boost_161_pthread_condition_variable.h b/clients/roscpp/include/boost_161_pthread_condition_variable.h
@@ -1,63 +1,21 @@
-#ifndef BOOST_THREAD_CONDITION_VARIABLE_PTHREAD_HPP
-#define BOOST_THREAD_CONDITION_VARIABLE_PTHREAD_HPP
+#ifndef BOOST_161_THREAD_CONDITION_VARIABLE_PTHREAD_HPP
+#define BOOST_161_THREAD_CONDITION_VARIABLE_PTHREAD_HPP
 // Distributed under the Boost Software License, Version 1.0. (See
 // accompanying file LICENSE_1_0.txt or copy at
 // http://www.boost.org/LICENSE_1_0.txt)
 // (C) Copyright 2007-10 Anthony Williams
 // (C) Copyright 2011-2012 Vicente J. Botet Escriba
 
 // make sure we include our backported version first!!
-// (before the system version might be included via some of the other header files)
 #include "boost_161_pthread_condition_variable_fwd.h"
 
-#include <boost/thread/pthread/timespec.hpp>
-#include <boost/thread/pthread/pthread_mutex_scoped_lock.hpp>
-#if defined BOOST_THREAD_PROVIDES_INTERRUPTIONS
-#include <boost/thread/pthread/thread_data.hpp>
-#endif
-//#include <boost/thread/pthread/condition_variable_fwd.hpp>
-#ifdef BOOST_THREAD_USES_CHRONO
-#include <boost/chrono/system_clocks.hpp>
-#include <boost/chrono/ceil.hpp>
-#endif
-#include <boost/thread/detail/delete.hpp>
+// include upstream
+#include <boost/thread/pthread/condition_variable.hpp>
 
 #include <boost/config/abi_prefix.hpp>
 
-namespace boost
+namespace boost_161
 {
-#if defined BOOST_THREAD_PROVIDES_INTERRUPTIONS
-    namespace this_thread
-    {
-        void BOOST_THREAD_DECL interruption_point();
-    }
-#endif
-
-    namespace thread_cv_detail
-    {
-        template<typename MutexType>
-        struct lock_on_exit
-        {
-            MutexType* m;
-
-            lock_on_exit():
-                m(0)
-            {}
-
-            void activate(MutexType& m_)
-            {
-                m_.unlock();
-                m=&m_;
-            }
-            ~lock_on_exit()
-            {
-                if(m)
-                {
-                    m->lock();
-                }
-           }
-        };
-    }
 
     inline void condition_variable::wait(unique_lock<mutex>& m)
     {
@@ -154,7 +112,7 @@ namespace boost
             {
                 boost::throw_exception(thread_resource_error(res, "boost::condition_variable_any::condition_variable_any() failed in pthread_mutex_init"));
             }
-            int const res2 = detail::monotonic_pthread_cond_init(cond);
+            int const res2 = detail_161::monotonic_pthread_cond_init(cond);
             if(res2)
             {
                 BOOST_VERIFY(!pthread_mutex_destroy(&internal_mutex));

diff --git a/clients/roscpp/include/boost_161_pthread_condition_variable_fwd.h b/clients/roscpp/include/boost_161_pthread_condition_variable_fwd.h
@@ -1,38 +1,22 @@
-#ifndef BOOST_THREAD_PTHREAD_CONDITION_VARIABLE_FWD_HPP
-#define BOOST_THREAD_PTHREAD_CONDITION_VARIABLE_FWD_HPP
+#ifndef BOOST_161_THREAD_PTHREAD_CONDITION_VARIABLE_FWD_HPP
+#define BOOST_161_THREAD_PTHREAD_CONDITION_VARIABLE_FWD_HPP
 // Distributed under the Boost Software License, Version 1.0. (See
 // accompanying file LICENSE_1_0.txt or copy at
 // http://www.boost.org/LICENSE_1_0.txt)
 // (C) Copyright 2007-8 Anthony Williams
 // (C) Copyright 2011-2012 Vicente J. Botet Escriba
 
-// define to check if this backported version was included
-#define USING_BACKPORTED_BOOST_CONDITION_VARIABLE 1
-
-#include <boost/assert.hpp>
-#include <boost/throw_exception.hpp>
-#include <pthread.h>
-#include <boost/thread/cv_status.hpp>
-#include <boost/thread/mutex.hpp>
-#include <boost/thread/lock_types.hpp>
-#include <boost/thread/thread_time.hpp>
-#include <boost/thread/pthread/timespec.hpp>
-#if defined BOOST_THREAD_USES_DATETIME
-#include <boost/thread/xtime.hpp>
-#endif
-
-#ifdef BOOST_THREAD_USES_CHRONO
-#include <boost/chrono/system_clocks.hpp>
-#include <boost/chrono/ceil.hpp>
-#endif
-#include <boost/thread/detail/delete.hpp>
-#include <boost/date_time/posix_time/posix_time_duration.hpp>
+// include upstream
+#include <boost/thread/pthread/condition_variable_fwd.hpp>
 
 #include <boost/config/abi_prefix.hpp>
 
-namespace boost
+namespace boost_161
 {
-  namespace detail {
+  using namespace boost;
+  namespace detail = boost::detail;
+
+  namespace detail_161 {
     inline int monotonic_pthread_cond_init(pthread_cond_t& cond) {
 
 #ifdef BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
@@ -100,7 +84,7 @@ namespace boost
                 boost::throw_exception(thread_resource_error(res, "boost::condition_variable::condition_variable() constructor failed in pthread_mutex_init"));
             }
 #endif
-            res = detail::monotonic_pthread_cond_init(cond);
+            res = detail_161::monotonic_pthread_cond_init(cond);
             if (res)
             {
 #if defined BOOST_THREAD_PROVIDES_INTERRUPTIONS

diff --git a/clients/roscpp/include/ros/callback_queue.h b/clients/roscpp/include/ros/callback_queue.h
@@ -37,22 +37,11 @@
 
 // check if we might need to include our own backported version boost::condition_variable
 // in order to use CLOCK_MONOTONIC for the condition variable
-// the include order here is important!
-#ifdef BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
-#include <boost/version.hpp>
-#if BOOST_VERSION < 106100
-// use backported version of boost condition variable, see https://svn.boost.org/trac/boost/ticket/6377
-#include "boost_161_condition_variable.h"
-#else // Boost version is 1.61 or greater and has the steady clock fixes
-#include <boost/thread/condition_variable.hpp>
-#endif
-#else // !BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
-#include <boost/thread/condition_variable.hpp>
-#endif // BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
+#include "ros/common.h"
+#include ROSCPP_BOOST_CONDITION_VARIABLE_HEADER
 
 #include "ros/callback_queue_interface.h"
 #include "ros/time.h"
-#include "common.h"
 
 #include <boost/shared_ptr.hpp>
 #include <boost/thread/mutex.hpp>
@@ -177,7 +166,7 @@ class ROSCPP_DECL CallbackQueue : public CallbackQueueInterface
   D_CallbackInfo callbacks_;
   size_t calling_;
   boost::mutex mutex_;
-  boost::condition_variable condition_;
+  ROSCPP_BOOST_CONDITION_VARIABLE condition_;
 
   boost::mutex id_info_mutex_;
   M_IDInfo id_info_;

diff --git a/clients/roscpp/include/ros/common.h.in b/clients/roscpp/include/ros/common.h.in
@@ -38,6 +38,7 @@
 #include "ros/serialized_message.h"
 
 #include <boost/shared_array.hpp>
+#include <boost/version.hpp>
 
 #define ROS_VERSION_MAJOR @roscpp_VERSION_MAJOR@
 #define ROS_VERSION_MINOR @roscpp_VERSION_MINOR@
@@ -48,6 +49,18 @@
 #define ROS_VERSION_GE(major1, minor1, patch1, major2, minor2, patch2) (ROS_VERSION_COMBINED(major1, minor1, patch1) >= ROS_VERSION_COMBINED(major2, minor2, patch2))
 #define ROS_VERSION_MINIMUM(major, minor, patch) ROS_VERSION_GE(ROS_VERSION_MAJOR, ROS_VERSION_MINOR, ROS_VERSION_PATCH, major, minor, patch)
 
+// check if we might need to include our own backported version boost::condition_variable
+// in order to use CLOCK_MONOTONIC for the condition variable and for the SteadyTimer
+#cmakedefine ROSCPP_USE_BACKPORTED_BOOST_CONDITION_VARIABLE_IMPLEMENTATION
+#ifdef ROSCPP_USE_BACKPORTED_BOOST_CONDITION_VARIABLE_IMPLEMENTATION
+  // use backported version of boost condition variable, see https://svn.boost.org/trac/boost/ticket/6377
+  #define ROSCPP_BOOST_CONDITION_VARIABLE_HEADER "boost_161_condition_variable.h"
+  #define ROSCPP_BOOST_CONDITION_VARIABLE boost_161::condition_variable
+#else
+  #define ROSCPP_BOOST_CONDITION_VARIABLE_HEADER <boost/thread/condition_variable.hpp>
+  #define ROSCPP_BOOST_CONDITION_VARIABLE boost::condition_variable
+#endif
+
 #include <ros/macros.h>
 
 // Import/export for windows dll's and visibility for gcc shared libraries.

diff --git a/clients/roscpp/include/ros/rosout_appender.h b/clients/roscpp/include/ros/rosout_appender.h
@@ -35,8 +35,12 @@
 #ifndef ROSCPP_ROSOUT_APPENDER_H
 #define ROSCPP_ROSOUT_APPENDER_H
 
+// check if we might need to include our own backported version boost::condition_variable
+// in order to use CLOCK_MONOTONIC for the condition variable
+#include "ros/common.h"
+#include ROSCPP_BOOST_CONDITION_VARIABLE_HEADER
+
 #include <ros/message_forward.h>
-#include "common.h"
 
 #include <boost/shared_ptr.hpp>
 #include <boost/weak_ptr.hpp>
@@ -73,7 +77,7 @@ class ROSCPP_DECL ROSOutAppender : public ros::console::LogAppender
   typedef std::vector<rosgraph_msgs::LogPtr> V_Log;
   V_Log log_queue_;
   boost::mutex queue_mutex_;
-  boost::condition_variable queue_condition_;
+  ROSCPP_BOOST_CONDITION_VARIABLE queue_condition_;
   bool shutting_down_;
   bool disable_topics_;
 

diff --git a/clients/roscpp/include/ros/service_server_link.h b/clients/roscpp/include/ros/service_server_link.h
@@ -35,7 +35,10 @@
 #ifndef ROSCPP_SERVICE_SERVER_LINK_H
 #define ROSCPP_SERVICE_SERVER_LINK_H
 
+// check if we might need to include our own backported version boost::condition_variable
+// in order to use CLOCK_MONOTONIC for the condition variable
 #include "ros/common.h"
+#include ROSCPP_BOOST_CONDITION_VARIABLE_HEADER
 
 #include <boost/thread/mutex.hpp>
 #include <boost/shared_array.hpp>
@@ -64,7 +67,7 @@ class ROSCPP_DECL ServiceServerLink : public boost::enable_shared_from_this<Serv
     SerializedMessage* resp_;
 
     bool finished_;
-    boost::condition_variable finished_condition_;
+    ROSCPP_BOOST_CONDITION_VARIABLE finished_condition_;
     boost::mutex finished_mutex_;
     boost::thread::id caller_thread_id_;
 

diff --git a/clients/roscpp/include/ros/timer_manager.h b/clients/roscpp/include/ros/timer_manager.h
@@ -30,18 +30,8 @@
 
 // check if we might need to include our own backported version boost::condition_variable
 // in order to use CLOCK_MONOTONIC for the SteadyTimer
-// the include order here is important!
-#ifdef BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
-#include <boost/version.hpp>
-#if BOOST_VERSION < 106100
-// use backported version of boost condition variable, see https://svn.boost.org/trac/boost/ticket/6377
-#include "boost_161_condition_variable.h"
-#else // Boost version is 1.61 or greater and has the steady clock fixes
-#include <boost/thread/condition_variable.hpp>
-#endif
-#else // !BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
-#include <boost/thread/condition_variable.hpp>
-#endif // BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
+#include "ros/common.h"
+#include ROSCPP_BOOST_CONDITION_VARIABLE_HEADER
 
 #include "ros/forwards.h"
 #include "ros/time.h"
@@ -126,7 +116,7 @@ class TimerManager
 
   V_TimerInfo timers_;
   boost::mutex timers_mutex_;
-  boost::condition_variable timers_cond_;
+  ROSCPP_BOOST_CONDITION_VARIABLE timers_cond_;
   volatile bool new_timer_;
 
   boost::mutex waiting_mutex_;
@@ -230,7 +220,13 @@ template<class T, class D, class E>
 TimerManager<T, D, E>::TimerManager() :
   new_timer_(false), id_counter_(0), thread_started_(false), quit_(false)
 {
-
+#if (BOOST_VERSION < 106700) && !defined(BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC)
+  ROS_ASSERT_MSG(false,
+                 "ros::TimerManager was instantiated by package " ROS_PACKAGE_NAME
+                 "without that BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC is defined! "
+                 "Be aware that timers might misbehave when system time jumps, "
+                 "e.g. due to network time corrections.");
+#endif
 }
 
 template<class T, class D, class E>

diff --git a/clients/roscpp/src/libros/callback_queue.cpp b/clients/roscpp/src/libros/callback_queue.cpp
@@ -32,23 +32,10 @@
  *  POSSIBILITY OF SUCH DAMAGE.
  */
 
-// Make sure we use CLOCK_MONOTONIC for the condition variable wait_for if not Apple.
-#ifndef __APPLE__
-#define BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
-#endif
-
 #include "ros/callback_queue.h"
 #include "ros/assert.h"
 #include <boost/scope_exit.hpp>
 
-// check if we have really included the backported boost condition variable
-// just in case someone messes with the include order...
-#if BOOST_VERSION < 106100
-#ifndef USING_BACKPORTED_BOOST_CONDITION_VARIABLE
-#error "needs boost version >= 1.61 or the backported headers!"
-#endif
-#endif
-
 namespace ros
 {
 

diff --git a/clients/roscpp/src/libros/internal_timer_manager.cpp b/clients/roscpp/src/libros/internal_timer_manager.cpp
@@ -25,22 +25,9 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */
 
-// Make sure we use CLOCK_MONOTONIC for the condition variable if not Apple.
-#if !defined(__APPLE__) && !defined(WIN32)
-#define BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
-#endif
-
 #include "ros/timer_manager.h"
 #include "ros/internal_timer_manager.h"
 
-// check if we have really included the backported boost condition variable
-// just in case someone messes with the include order...
-#if BOOST_VERSION < 106100
-#ifndef USING_BACKPORTED_BOOST_CONDITION_VARIABLE
-#error "steady timer needs boost version >= 1.61 or the backported headers!"
-#endif
-#endif
-
 namespace ros
 {
 

diff --git a/clients/roscpp/src/libros/steady_timer.cpp b/clients/roscpp/src/libros/steady_timer.cpp
@@ -25,22 +25,9 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */
 
-// Make sure we use CLOCK_MONOTONIC for the condition variable if not Apple.
-#if !defined(__APPLE__) && !defined(WIN32)
-#define BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC
-#endif
-
 #include "ros/steady_timer.h"
 #include "ros/timer_manager.h"
 
-// check if we have really included the backported boost condition variable
-// just in case someone messes with the include order...
-#if BOOST_VERSION < 106100
-#ifndef USING_BACKPORTED_BOOST_CONDITION_VARIABLE
-#error "steady timer needs boost version >= 1.61 or the backported headers!"
-#endif
-#endif
-
 namespace ros
 {