From 134c3239584217f628b5949498bed90bb3e3f50a Mon Sep 17 00:00:00 2001 From: "Vicente J. Botet Escriba" Date: Fri, 13 Sep 2013 19:01:48 +0000 Subject: [PATCH] Thread: merge scoped_thread constr + condition_variable timed wait issues on windows + doc typos. [SVN r85663] --- doc/changes.qbk | 10 ++- doc/configuration.qbk | 8 +- doc/external_locking.qbk | 2 +- doc/scoped_thread.qbk | 4 +- doc/sync_tutorial.qbk | 4 +- doc/thread_ref.qbk | 10 +-- example/not_interleaved.cpp | 4 +- example/producer_consumer.cpp | 6 +- example/producer_consumer_bounded.cpp | 6 +- example/scoped_thread.cpp | 8 +- include/boost/thread/scoped_thread.hpp | 10 +-- .../boost/thread/win32/condition_variable.hpp | 19 ++++- test/Jamfile.v2 | 4 +- test/test_9079_a.cpp | 57 +++++++++++++ test/test_9079_b.cpp | 81 +++++++++++++++++++ 15 files changed, 198 insertions(+), 35 deletions(-) create mode 100644 test/test_9079_a.cpp create mode 100644 test/test_9079_b.cpp diff --git a/doc/changes.qbk b/doc/changes.qbk index c7c798131..ebbf9d5bc 100644 --- a/doc/changes.qbk +++ b/doc/changes.qbk @@ -8,7 +8,6 @@ [section:changes History] -[/] [heading Version 4.2.0 - boost 1.55] [*New Features:] @@ -17,15 +16,20 @@ * [@http://svn.boost.org/trac/boost/ticket/8518 #8518] Synchro: Add a latch class. * [@http://svn.boost.org/trac/boost/ticket/8519 #8519] Synchro: Update class barrier with a completion function. -* [@http://svn.boost.org/trac/boost/ticket/8615 #8615] Async: Replace make_future/make_shared_future by make_ready_future. * [@http://svn.boost.org/trac/boost/ticket/8515 #8515] Async: Add shared_future::then. +* [@http://svn.boost.org/trac/boost/ticket/8615 #8615] Async: Replace make_future/make_shared_future by make_ready_future. * [@http://svn.boost.org/trac/boost/ticket/8627 #8627] Async: Add future<>::unwrap and unwrapping constructor. * [@http://svn.boost.org/trac/boost/ticket/8677 #8677] Async: Add future<>::get_or. * [@http://svn.boost.org/trac/boost/ticket/8678 #8678] Async: Add future<>::fallback_to. +* [@http://svn.boost.org/trac/boost/ticket/8891 #8891] upgrade_to_unique_lock: missing mutex() function + [*Fixed Bugs:] -[/] +* [@http://svn.boost.org/trac/boost/ticket/8931 #8931] Typos in external_locking reference +* [@http://svn.boost.org/trac/boost/ticket/9029 #9029] Misprint in documentation +* [@http://svn.boost.org/trac/boost/ticket/9037 #9037] [thread] gcc -Wshadow gives warnings in condition_variable{,_fwd}.hpp +* [@http://svn.boost.org/trac/boost/ticket/9041 #9041] [thread] Boost.Thread DSO's may need to link with Boost.Atomic [heading Version 4.1.0 - boost 1.54] diff --git a/doc/configuration.qbk b/doc/configuration.qbk index 43f3290af..83c4a42af 100644 --- a/doc/configuration.qbk +++ b/doc/configuration.qbk @@ -91,15 +91,15 @@ Define `BOOST_THREAD_DONT_USE_ATOMIC ` if you don't want to use Boost.Atomic or The following operators are deprecated: -* `boost::thread::oprator==` -* `boost::thread::oprator!=` +* `boost::thread::operator==` +* `boost::thread::operator!=` When `BOOST_THREAD_PROVIDES_THREAD_EQ` is defined Boost.Thread provides these deprecated feature. Use instead -* `boost::thread::id::oprator==` -* `boost::thread::id::oprator!=` +* `boost::thread::id::operator==` +* `boost::thread::id::operator!=` [warning This is a breaking change respect to version 1.x.] diff --git a/doc/external_locking.qbk b/doc/external_locking.qbk index c9e722c66..6eab28ddd 100644 --- a/doc/external_locking.qbk +++ b/doc/external_locking.qbk @@ -482,7 +482,7 @@ We need a way to transfer the ownership from the `unique_lock` to a `strict_lock In order to make this code compilable we need to store either a Lockable or a `unique_lock` reference depending on the constructor. Store which kind of reference we have stored,and in the destructor call either to the Lockable `unlock` or restore the ownership. -This seams too complicated to me. Another possibility is to define a nested strict lock class. The drawback is that instead of having only one strict lock we have two and we need either to duplicate every function taking a `strict_lock` or make these function templates functions. The problem with template functions is that we don't profit anymore of the C++ type system. We must add some static metafunction that check that the Locker parameter is a strict lock. The problem is that we can not really check this or can we?. The `is_strict_lock` metafunction must be specialized by the strict lock developer. We need to belive it "sur parole". The advantage is that now we can manage with more than two strict locks without changing our code. Ths is really nice. +This seams too complicated to me. Another possibility is to define a nested strict lock class. The drawback is that instead of having only one strict lock we have two and we need either to duplicate every function taking a `strict_lock` or make these function templates functions. The problem with template functions is that we don't profit anymore of the C++ type system. We must add some static metafunction that check that the Locker parameter is a strict lock. The problem is that we can not really check this or can we?. The `is_strict_lock` metafunction must be specialized by the strict lock developer. We need to belive it "sur parole". The advantage is that now we can manage with more than two strict locks without changing our code. This is really nice. Now we need to state that both classes are `strict_lock`s. diff --git a/doc/scoped_thread.qbk b/doc/scoped_thread.qbk index 435721123..938f65089 100644 --- a/doc/scoped_thread.qbk +++ b/doc/scoped_thread.qbk @@ -190,7 +190,7 @@ This wrapper can be used to join the thread before destroying it seems a natural explicit scoped_thread(thread&& th) noexcept; template - explicit strict_scoped_thread(F&&, Args&&...); + explicit scoped_thread(F&&, Args&&...); ~scoped_thread(); @@ -323,7 +323,7 @@ any) to `*this`. [section:call_constructor Move Constructor from a Callable] template - explicit strict_scoped_thread(F&&, Args&&...); + explicit scoped_thread(F&&, Args&&...); [variablelist diff --git a/doc/sync_tutorial.qbk b/doc/sync_tutorial.qbk index 15e4044e8..d1ba29c50 100644 --- a/doc/sync_tutorial.qbk +++ b/doc/sync_tutorial.qbk @@ -23,9 +23,9 @@ In addition to the C++11 standard locks, Boost.Thread provides other locks and s In particular, the library provides some lock factories. template - auto with_lock_guard(Lockable& m, Function f) -> decltype(fn()) + auto with_lock_guard(Lockable& m, Function f) -> decltype(f()) { - auto&& _ = boost::make_lock_guard(f); + auto&& _ = boost::make_lock_guard(m); f(); } diff --git a/doc/thread_ref.qbk b/doc/thread_ref.qbk index 9deaabdce..e00601651 100644 --- a/doc/thread_ref.qbk +++ b/doc/thread_ref.qbk @@ -568,7 +568,7 @@ any) to `*this`. [variablelist -[[Requires:] [`Callable` must by Copyable and `func()` must be a valid expression.]] +[[Requires:] [`Callable` must be Copyable and `func()` must be a valid expression.]] [[Effects:] [`func` is copied into storage managed internally by the thread library, and that copy is invoked on a newly-created thread of execution. If this invocation results in an exception being propagated into the internals of the thread library that is @@ -595,7 +595,7 @@ not of type __thread_interrupted__, then `std::terminate()` will be called. Any [variablelist -[[Preconditions:] [`Callable` must by copyable.]] +[[Preconditions:] [`Callable` must be copyable.]] [[Effects:] [`func` is copied into storage managed internally by the thread library, and that copy is invoked on a newly-created thread of execution with the specified attributes. If this invocation results in an exception being propagated into the internals of the thread library that is @@ -623,7 +623,7 @@ If the attributes declare the native thread as detached, the boost::thread will [variablelist -[[Preconditions:] [`Callable` must by Movable.]] +[[Preconditions:] [`Callable` must be Movable.]] [[Effects:] [`func` is moved into storage managed internally by the thread library, and that copy is invoked on a newly-created thread of execution. If this invocation results in an exception being propagated into the internals of the thread library that is @@ -650,7 +650,7 @@ not of type __thread_interrupted__, then `std::terminate()` will be called. Any [variablelist -[[Preconditions:] [`Callable` must by copyable.]] +[[Preconditions:] [`Callable` must be copyable.]] [[Effects:] [`func` is copied into storage managed internally by the thread library, and that copy is invoked on a newly-created thread of execution with the specified attributes. If this invocation results in an exception being propagated into the internals of the thread library that is @@ -679,7 +679,7 @@ If the attributes declare the native thread as detached, the boost::thread will [variablelist -[[Preconditions:] [`F` and each `A`n must by copyable or movable.]] +[[Preconditions:] [`F` and each `A`n must be copyable or movable.]] [[Effects:] [As if [link thread.thread_management.thread.callable_constructor diff --git a/example/not_interleaved.cpp b/example/not_interleaved.cpp index 2cd9f7ba4..ad90fd1f5 100644 --- a/example/not_interleaved.cpp +++ b/example/not_interleaved.cpp @@ -44,8 +44,8 @@ int main() externally_locked_stream mcout(std::cout, terminal_mutex); externally_locked_stream mcin(std::cin, terminal_mutex); - scoped_thread<> t1(thread(use_cerr, boost::ref(mcerr))); - scoped_thread<> t2(thread(use_cout, boost::ref(mcout))); + scoped_thread<> t1(boost::thread(use_cerr, boost::ref(mcerr))); + scoped_thread<> t2(boost::thread(use_cout, boost::ref(mcout))); this_thread::sleep_for(chrono::seconds(2)); std::string nm; { diff --git a/example/producer_consumer.cpp b/example/producer_consumer.cpp index 7b7df1494..483216a80 100644 --- a/example/producer_consumer.cpp +++ b/example/producer_consumer.cpp @@ -110,9 +110,9 @@ int main() { mcout << "begin of main" << std::endl; - scoped_thread<> t11(thread(producer, boost::ref(mcerr), boost::ref(sbq))); - scoped_thread<> t12(thread(producer, boost::ref(mcerr), boost::ref(sbq))); - scoped_thread<> t2(thread(consumer, boost::ref(mcout), boost::ref(sbq))); + scoped_thread<> t11(boost::thread(producer, boost::ref(mcerr), boost::ref(sbq))); + scoped_thread<> t12(boost::thread(producer, boost::ref(mcerr), boost::ref(sbq))); + scoped_thread<> t2(boost::thread(consumer, boost::ref(mcout), boost::ref(sbq))); this_thread::sleep_for(chrono::seconds(1)); diff --git a/example/producer_consumer_bounded.cpp b/example/producer_consumer_bounded.cpp index c11965d80..76c060ef4 100644 --- a/example/producer_consumer_bounded.cpp +++ b/example/producer_consumer_bounded.cpp @@ -110,9 +110,9 @@ int main() { mcout << "begin of main" << std::endl; - scoped_thread<> t11(thread(producer, boost::ref(mcerr), boost::ref(sbq))); - scoped_thread<> t12(thread(producer, boost::ref(mcerr), boost::ref(sbq))); - scoped_thread<> t2(thread(consumer, boost::ref(mcout), boost::ref(sbq))); + scoped_thread<> t11(boost::thread(producer, boost::ref(mcerr), boost::ref(sbq))); + scoped_thread<> t12(boost::thread(producer, boost::ref(mcerr), boost::ref(sbq))); + scoped_thread<> t2(boost::thread(consumer, boost::ref(mcout), boost::ref(sbq))); this_thread::sleep_for(chrono::seconds(1)); diff --git a/example/scoped_thread.cpp b/example/scoped_thread.cpp index ec2ca81fc..f494bf2f7 100644 --- a/example/scoped_thread.cpp +++ b/example/scoped_thread.cpp @@ -13,6 +13,9 @@ void do_something(int& i) { ++i; } +void f(int, int) +{ +} struct func { @@ -81,7 +84,10 @@ int main() do_something_in_current_thread(); } - + { + boost::scoped_thread<> g( f, 1, 2 ); + do_something_in_current_thread(); + } return 0; } diff --git a/include/boost/thread/scoped_thread.hpp b/include/boost/thread/scoped_thread.hpp index 3f43d4693..e7a00f692 100644 --- a/include/boost/thread/scoped_thread.hpp +++ b/include/boost/thread/scoped_thread.hpp @@ -47,9 +47,8 @@ namespace boost * */ #if ! defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) - template - explicit strict_scoped_thread(BOOST_THREAD_FWD_REF(F) f, BOOST_THREAD_FWD_REF(Args)... args, - typename disable_if::type, thread>, dummy* >::type=0) : + template ::type, thread>, dummy* >::type> + explicit strict_scoped_thread(BOOST_THREAD_FWD_REF(F) f, BOOST_THREAD_FWD_REF(Args)... args) : t_(boost::forward(f), boost::forward(args)...) {} #else template @@ -138,9 +137,8 @@ namespace boost */ #if ! defined(BOOST_NO_CXX11_VARIADIC_TEMPLATES) - template - explicit scoped_thread(BOOST_THREAD_FWD_REF(F) f, BOOST_THREAD_FWD_REF(Args)... args, - typename disable_if::type, thread>, dummy* >::type=0) : + template ::type, thread>, dummy* >::type> + explicit scoped_thread(BOOST_THREAD_FWD_REF(F) f, BOOST_THREAD_FWD_REF(Args)... args) : t_(boost::forward(f), boost::forward(args)...) {} #else template diff --git a/include/boost/thread/win32/condition_variable.hpp b/include/boost/thread/win32/condition_variable.hpp index 64a816068..d07ddcbe0 100644 --- a/include/boost/thread/win32/condition_variable.hpp +++ b/include/boost/thread/win32/condition_variable.hpp @@ -366,7 +366,11 @@ namespace boost const chrono::time_point& t) { using namespace chrono; - do_wait(lock, ceil(t-Clock::now()).count()); + chrono::time_point now = Clock::now(); + if (t<=now) { + return cv_status::timeout; + } + do_wait(lock, ceil(t-now).count()); return Clock::now() < t ? cv_status::no_timeout : cv_status::timeout; } @@ -378,6 +382,10 @@ namespace boost const chrono::duration& d) { using namespace chrono; + if (d<=chrono::duration::zero()) { + return cv_status::timeout; + } + steady_clock::time_point c_now = steady_clock::now(); do_wait(lock, ceil(d).count()); return steady_clock::now() - c_now < d ? cv_status::no_timeout : @@ -479,7 +487,11 @@ namespace boost const chrono::time_point& t) { using namespace chrono; - do_wait(lock, ceil(t-Clock::now()).count()); + chrono::time_point now = Clock::now(); + if (t<=now) { + return cv_status::timeout; + } + do_wait(lock, ceil(t-now).count()); return Clock::now() < t ? cv_status::no_timeout : cv_status::timeout; } @@ -491,6 +503,9 @@ namespace boost const chrono::duration& d) { using namespace chrono; + if (d<=chrono::duration::zero()) { + return cv_status::timeout; + } steady_clock::time_point c_now = steady_clock::now(); do_wait(lock, ceil(d).count()); return steady_clock::now() - c_now < d ? cv_status::no_timeout : diff --git a/test/Jamfile.v2 b/test/Jamfile.v2 index 4ba5efde0..fe6627613 100644 --- a/test/Jamfile.v2 +++ b/test/Jamfile.v2 @@ -792,7 +792,9 @@ rule thread-compile ( sources : reqs * : name ) #[ thread-run test_8600.cpp ] #[ thread-run test_8943.cpp ] #[ thread-run test_8960.cpp ] - + [ thread-run test_9079_a.cpp ] + [ thread-run test_9079_b.cpp ] + ; } diff --git a/test/test_9079_a.cpp b/test/test_9079_a.cpp new file mode 100644 index 000000000..cfe83366a --- /dev/null +++ b/test/test_9079_a.cpp @@ -0,0 +1,57 @@ +// Copyright (C) 2013 Vicente Botet +// +// 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) + +// A + +//#include +#include +#include +#include + +//#if !defined(BOOST_NO_CXX11_ALIGNAS) +//#error +//# define BOOST_ALIGNMENT2(x) alignas(x) +//#elif defined(_MSC_VER) +//#error +//# define BOOST_ALIGNMENT2(x) __declspec(align(x)) +//#elif defined(__GNUC__) +//#error +//# define BOOST_ALIGNMENT(x) __attribute__ ((__aligned__(x))) +//#else +//#error +//# define BOOST_NO_ALIGNMENT2 +//# define BOOST_ALIGNMENT2(x) +//#endif + +typedef boost::chrono::high_resolution_clock Clock; +typedef Clock::time_point TimePoint; + +inline TimePoint real_time_now() +{ + return Clock::now(); +} + +int main() +{ + using namespace boost::chrono; + + boost::condition_variable m_task_spawn_condition; + + boost::mutex main_thread_mutex; + boost::unique_lock < boost::mutex > main_thread_lock(main_thread_mutex); + + //BOOST_LOG_TRIVIAL(info) << "[TaskScheduler::run_and_wait] Scheduling loop - BEGIN"; + + //while (true) + { + static const milliseconds TIME_BACK = milliseconds(1); + m_task_spawn_condition.wait_until( + main_thread_lock, + real_time_now() - TIME_BACK); // wait forever + m_task_spawn_condition.wait_for( main_thread_lock, - TIME_BACK ); // same problem + //BOOST_LOG_TRIVIAL(trace) << "TICK"; + } + +} diff --git a/test/test_9079_b.cpp b/test/test_9079_b.cpp new file mode 100644 index 000000000..7a59202be --- /dev/null +++ b/test/test_9079_b.cpp @@ -0,0 +1,81 @@ +// Copyright (C) 2013 Vicente Botet +// +// 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) + +// B + +#include +//#include +#include +#include +#include + +typedef boost::chrono::high_resolution_clock Clock; +typedef Clock::time_point TimePoint; + +inline TimePoint real_time_now() +{ + return Clock::now(); +} + +class Foo { + boost::atomic m_is_exiting; + TimePoint m_next_tick_time; + +public: + + bool is_exiting() const + { + return m_is_exiting; + } + + TimePoint spawn_tasks() // note that in my app, this call takes more time than here + { + using namespace boost::chrono; + const TimePoint now = real_time_now(); + + if (m_next_tick_time < now) { + m_next_tick_time = now + seconds(1); + //BOOST_LOG_TRIVIAL(info) << "TICK!"; + } + + return m_next_tick_time; + } + +}; + +int main() +{ + using namespace boost::chrono; + static const milliseconds MIN_TIME_TASKS_SPAWN_FREQUENCY = milliseconds(1); + //microseconds(1); // THE SHORTER THE QUICKER TO REPRODUCE THE BUG + + boost::condition_variable m_task_spawn_condition; + Foo foo; + + boost::mutex main_thread_mutex; + boost::unique_lock < boost::mutex > main_thread_lock(main_thread_mutex); + + //BOOST_LOG_TRIVIAL(info) << "[TaskScheduler::run_and_wait] Scheduling loop - BEGIN"; + + while (!foo.is_exiting()) { + const TimePoint next_task_spawn_time = foo.spawn_tasks(); + + const TimePoint now = real_time_now(); + const TimePoint next_minimum_spawn_time = now + MIN_TIME_TASKS_SPAWN_FREQUENCY; + const TimePoint next_spawn_time = next_task_spawn_time > TimePoint() + && next_task_spawn_time < next_minimum_spawn_time + ? next_task_spawn_time : next_minimum_spawn_time; + + const TimePoint::duration wait_time = next_spawn_time - now; + if (wait_time > wait_time.zero()) { + // BOOST_LOG_TRIVIAL(trace) << "WAIT TIME: " << wait_time; // UNCOMMENT THIS: MAKES IT WORKS. WAT?????? + m_task_spawn_condition.wait_until( + main_thread_lock, + next_spawn_time); // DON'T WORK: WILL WAIT IF next_spawn_time is too close! + } + + } + +}