Use an internal implementation of boost::condition_variable with monotonic clock #1932
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lock to avoid ODR violation
…os/internal/condition_variable.h
|
@ros-pull-request-builder retest this please |
Closed
meyerj
added a commit
to meyerj/ros_comm
that referenced
this pull request
Jul 31, 2020
…tonic clock (ros#1932) * Use an internal implementation of condition_variable with monotonic clock to avoid ODR violation * Fix build of ros/internal/condition_variable.h for Boost <1.65 * Fix "static_assert with no message is a C++17 extension" warning in ros/internal/condition_variable.h
dirk-thomas
added a commit
that referenced
this pull request
Aug 3, 2020
…tonic clock [kinetic-devel] (#2011) * Drop custom implementation of boost::condition_variable to fix busy-wait spinning (#1878) * roscpp: fix potential busy-wait loop caused by backported Boost condition_variable (fix #1343) #1014 and #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. * roscpp: use boost::condition_variable::wait_for() instead of deprecated timed_wait() This fixes ROS timers in combination with 2c18b9f. The timer callbacks were not called because the TimerManager's thread function blocked indefinitely on boost::condition_variable::timed_wait(). Relative timed_wait() uses the system clock (boost::get_system_time()) unconditionally to calculate the absolute timestamp for do_wait_until(). If the condition variable has been initialized with BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC, it compares this timestamp with the monotonic clock and therefore blocks. This issue has been reported in https://svn.boost.org/trac10/ticket/12728 and will not be fixed. The timed_wait interface is apparently deprecated. * roscpp: do not use boost_161_condition_variable.h on Windows (untested) * roscpp: remove specialized implementation of TimerManager<T,D,E>::threadFunc() in steady_timer.cpp The updated generic definition in timer_manager.h should do the same with a minor update. In all cases we can call boost::condition_variable::wait_until() with an absolute time_point of the respective clock. The conversion from system_clock to steady_clock for Time and WallTime is done internally in boost::condition_variable::wait_until(lock_type& lock, const chrono::time_point<Clock, Duration>& t). * fix namespaces * add more explicit namespaces * add missing ns * roscpp: fixed Boost version check in CMakeLists.txt find_package(Boost) has to come before checking the Boost version. Otherwise BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC was not defined which triggered the assertion in timer_manager.h:240. Since Boost 1.67 BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC became the default if the platform supports it and the macro is not defined anymore. Instead, check for BOOST_THREAD_INTERNAL_CLOCK_IS_MONO. * roscpp: replace ROSCPP_BOOST_CONDITION_VARIABLE and ROSCPP_BOOST_CONDITION_VARIABLE_HEADER macros by a typedef in internal_condition_variable.h * Remove copy of boost::condition_variable implementation from Boost 1.61 in namespace boost_161 * Revert some changes in include directives and in CMakeLists.txt to minimize the diff to melodic-devel Addresses #1878 (review). * use wait_for(), remove TimerManagerTraits * Revert "use wait_for(), remove TimerManagerTraits" This reverts commit 2a67cf6. Co-authored-by: Antoine Hoarau <703240+ahoarau@users.noreply.github.com> Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> * Use an internal implementation of boost::condition_variable with monotonic clock (#1932) * Use an internal implementation of condition_variable with monotonic clock to avoid ODR violation * Fix build of ros/internal/condition_variable.h for Boost <1.65 * Fix "static_assert with no message is a C++17 extension" warning in ros/internal/condition_variable.h * roscpp: fix ros/internal/condition_variable.h for pre-C++11 compilers Co-authored-by: Antoine Hoarau <703240+ahoarau@users.noreply.github.com> Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
|
I think this does not compile on OS X unfortunately: http://robostack.net:8000/RoboStack/vinca/94/2/2 |
|
@meyerj Can you take a look at this and work on a pull request to address the regression? |
This was referenced Aug 4, 2020
|
I created #2019 to not forget about these comments on a closed ticket. |
| boost::unique_lock<boost::mutex> &lock, | ||
| const boost::chrono::time_point<Clock, Duration> &t) | ||
| { | ||
| using namespace boost::chrono; |
There was a problem hiding this comment.
these instances of using namespace boost::chrono in these header functions are causing build failures in my downstream packages, because it introduces potential name collisions with the same types in std::chrono which are included indirectly as well by the packages
Though this is probably an indicator of poor using-hygiene on the part of one of my dependencies, I'd still say it's a regression in Melodic. Is there any reason this code must use using, or could it call boost::chrono::steady_clock etc in these functions?
There was a problem hiding this comment.
Is there any reason this code must use
using, or could it callboost::chrono::steady_clocketc in these functions?
I don't think there is technical reason why using was used. Please consider to create a pull request to change it to the fully qualified types and get rid of using namespace in the header.
There was a problem hiding this comment.
Sounds good - I've opened #2020 to follow up
There was a problem hiding this comment.
We ran into the same problem.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up on and partially revert #1878:
Unfortunately @ahoarau was right in #1878 (comment): The patch did not solve the issue with busy-wait spinning or non-functional timers completely, because the definition of
boost::condition_variable()provided by different compilation units might still have been compiled with non-matching settings ofBOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC. So even with the patch the eventually conflicting definitions of the symbol is an ODR violation and can cause undefined behavior, depending on the compiler internals and flags.For example,
libtf2_ros.sofrom latestros-melodic-tf2-rosprovides a weak symbolboost::condition_variable()which was compiled without the macroBOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC. Therefore, as soon aslibtf2_ros.sois linked into the process all condition variables are constructed with the standard system clock as their internal clock. The wait calls within roscpp, e.g. for timers, which expect that condition variables are configured with CLOCK_MONOTONIC internally, become no-ops then.While the problem has finally been solved in Boost 1.67 and the remaining checks for older Boost versions have been completely removed in #1903 for noetic, I propose to partially revert #1878 for melodic and older, and to re-add a custom stripped down version of
boost::condition_variableasros::internal::condition_variable_monotonic. This is more or less what I proposed in my earlier pull request #1651, but rebased on the latestmelodic-develbranch and simplified. The copy uses a different namespace and does not respectBOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC. It uses the monotonic clock unconditionally. This custom implementation is only selected for platforms that support it and ifBOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONICorBOOST_THREAD_INTERNAL_CLOCK_IS_MONOare not already defined by Boost itself (and therefore would be effective for all compilation units). Otherwiseros::internal::condition_variable_monotonicis an alias forboost::condition_variable: