Boost C++ Libraries: Ticket #10967: Timed wait points inconsistently convert relative to absolute waits

owner, status changed

viboes — Sun, 25 Jan 2015 09:09:33 GMT

owner changed from Anthony Williams to viboes
status new → assigned

Thanks for pointing out this incoherency. I will check this asap.

viboes — Sun, 25 Jan 2015 10:06:58 GMT

I've created a branch to fix this issue

https://github.com/boostorg/thread/tree/fix/10967_inconsistent_abs_rel_time

Niall, I'm unable to check it. Would you like that I commit some changes I think that could be done, or do you prefer I post them here?

viboes — Sun, 25 Jan 2015 10:10:08 GMT

E.g. it would be better to rename do_try_join_until to do_try_join_for. This would not fix anything, but it will make things clearer.

diff --git a/include/boost/thread/detail/thread.hpp b/include/boost/thread/detail/thread.hpp
index b80eacf..2774eb0 100644
--- a/include/boost/thread/detail/thread.hpp
+++ b/include/boost/thread/detail/thread.hpp
@@ -472,7 +472,7 @@ namespace boost
         bool try_join_for(const chrono::duration<Rep, Period>& rel_time)
         {
           chrono::milliseconds rel_time2= chrono::ceil<chrono::milliseconds>(rel_time);
-          return do_try_join_until(rel_time2.count());
+          return do_try_join_for(rel_time2.count());
         }
 #else
         template <class Rep, class Period>
@@ -504,19 +504,16 @@ namespace boost
 #endif
 #if defined(BOOST_THREAD_PLATFORM_WIN32)
     private:
-        bool do_try_join_until_noexcept(uintmax_t milli, bool& res);
-        inline bool do_try_join_until(uintmax_t milli);
+        bool do_try_join_for_noexcept(uintmax_t milli, bool& res);
+        inline bool do_try_join_for(uintmax_t milli);
     public:
         bool timed_join(const system_time& abs_time);
-        //{
-        //  return do_try_join_until(get_milliseconds_until(wait_until));
-        //}
 #ifdef BOOST_THREAD_USES_CHRONO
         bool try_join_until(const chrono::time_point<chrono::system_clock, chrono::nanoseconds>& tp)
         {
           chrono::milliseconds rel_time= chrono::ceil<chrono::milliseconds>(tp-chrono::system_clock::now());
-          return do_try_join_until(rel_time.count());
+          return do_try_join_for(rel_time.count());
         }
 #endif
@@ -769,9 +766,6 @@ namespace boost
 #ifdef BOOST_THREAD_PLATFORM_PTHREAD
     bool thread::do_try_join_until(struct timespec const &timeout)
-#else
-    bool thread::do_try_join_until(uintmax_t timeout)
-#endif
     {
         if (this_thread::get_id() == get_id())
           boost::throw_exception(thread_resource_error(static_cast<int>(system::errc::resource_deadlock_would_occur), "boost thread: trying joining itself"));
@@ -788,6 +782,25 @@ namespace boost
           );
         }
     }
+#else
+    bool thread::do_try_join_for(uintmax_t timeout)
+    {
+        if (this_thread::get_id() == get_id())
+          boost::throw_exception(thread_resource_error(static_cast<int>(system::errc::resource_deadlock_would_occur), "boost thread: trying joining itself"));
+        bool res;
+        if (do_try_join_for_noexcept(timeout, res))
+        {
+          return res;
+        }
+        else
+        {
+          BOOST_THREAD_THROW_ELSE_RETURN(
+            (thread_resource_error(static_cast<int>(system::errc::invalid_argument), "boost thread: thread not joinable")),
+            false
+          );
+        }
+    }
+#endif
 #if !defined(BOOST_NO_IOSTREAM) && defined(BOOST_NO_MEMBER_TEMPLATE_FRIENDS)
     template<class charT, class traits>
diff --git a/src/win32/thread.cpp b/src/win32/thread.cpp
index e02124f..2d9af0e 100644
--- a/src/win32/thread.cpp
+++ b/src/win32/thread.cpp
@@ -44,7 +44,7 @@
 #include <wrl\ftm.h>
 #include <windows.system.threading.h>
 #pragma comment(lib, "runtimeobject.lib")
-#endif
+#endif
 namespace boost
 {
@@ -198,7 +198,7 @@ namespace boost
     namespace detail
     {
         std::atomic_uint threadCount;
-
+
         bool win32::scoped_winrt_thread::start(thread_func address, void *parameter, unsigned int *thrdId)
         {
             Microsoft::WRL::ComPtr<ABI::Windows::System::Threading::IThreadPoolStatics> threadPoolFactory;
@@ -220,7 +220,7 @@ namespace boost
             m_completionHandle = completionHandle;
             // Create new work item.
-            Microsoft::WRL::ComPtr<ABI::Windows::System::Threading::IWorkItemHandler> workItem =
+            Microsoft::WRL::ComPtr<ABI::Windows::System::Threading::IWorkItemHandler> workItem =
                 Microsoft::WRL::Callback<Microsoft::WRL::Implements<Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::ClassicCom>, ABI::Windows::System::Threading::IWorkItemHandler, Microsoft::WRL::FtmBase>>
                 ([address, parameter, completionHandle](ABI::Windows::Foundation::IAsyncAction *)
             {
@@ -346,7 +346,7 @@ namespace boost
         return true;
 #endif
     }
-
+
     bool thread::start_thread_noexcept(const attributes& attr)
     {
 #if BOOST_PLAT_WINDOWS_RUNTIME
@@ -367,7 +367,7 @@ namespace boost
       return true;
 #endif
     }
-
+
     thread::thread(detail::thread_data_ptr data):
         thread_info(data)
     {}
@@ -469,10 +469,10 @@ namespace boost
 #if defined BOOST_THREAD_USES_DATETIME
     bool thread::timed_join(boost::system_time const& wait_until)
     {
-      return do_try_join_until(get_milliseconds_until(wait_until));
+      return do_try_join_for(get_milliseconds_until(wait_until));
     }
 #endif
-    bool thread::do_try_join_until_noexcept(uintmax_t milli, bool& res)
+    bool thread::do_try_join_for_noexcept(uintmax_t milli, bool& res)
     {
       detail::thread_data_ptr local_thread_info=(get_thread_info)();
       if(local_thread_info)

viboes — Sun, 25 Jan 2015 10:12:08 GMT

The following code seems to be using do_wait incorrectly, it gives a relative instead of an absolute time, isn't it?

        template <class Clock, class Duration>
        cv_status
        wait_until(
                unique_lock<mutex>& lock,
                const chrono::time_point<Clock, Duration>& t)
        {
          using namespace chrono;
          chrono::time_point<Clock, Duration> now = Clock::now();
          if (t<=now) {
            return cv_status::timeout;
          }
          do_wait(lock, ceil<milliseconds>(t-now).count());
          return Clock::now() < t ? cv_status::no_timeout :
                                             cv_status::timeout;
        }
        template <class Rep, class Period>
        cv_status
        wait_for(
                unique_lock<mutex>& lock,
                const chrono::duration<Rep, Period>& d)
        {
          using namespace chrono;
          if (d<=chrono::duration<Rep, Period>::zero()) {
            return cv_status::timeout;
          }
          steady_clock::time_point c_now = steady_clock::now();
          do_wait(lock, ceil<milliseconds>(d).count());
          return steady_clock::now() - c_now < d ? cv_status::no_timeout :
                                                   cv_status::timeout;
        }

owner, status changed

viboes — Sun, 25 Jan 2015 10:14:08 GMT

owner changed from viboes to Niall Douglas
status assigned → new

viboes — Sun, 25 Jan 2015 10:17:44 GMT

I've just seen your post. Adding the references you have identified here

Some references to code:

Always uses absolute: https://github.com/boostorg/thread/blob/develop/include/boost/thread/win32/condition_variable.hpp#L92

Always uses relative: https://github.com/boostorg/thread/blob/develop/include/boost/thread/win32/basic_timed_mutex.hpp#L190

Always uses relative: https://github.com/boostorg/thread/blob/develop/include/boost/thread/v2/thread.hpp#L60

Always uses absolute: https://github.com/boostorg/thread/blob/develop/include/boost/thread/detail/thread.hpp#L551

Niall Douglas — Sun, 25 Jan 2015 16:44:05 GMT

I think the API naming is indeed confusing. We have four situations for timed waits:

Wait until the system clock says some hour and date. If the system clock is adjusted, I want my wait adjusted. This is the same as wait_until(system_clock).

Wait until the steady clock has some deadline value. Adjustments of system clock have no effect. This is the same as wait_until(steady_clock).

Wait for X ticks of the steady clock to occur. Adjustments of system clock have no effect. This is the same as wait_for(steady_clock).

From my reading of the C++ standard, wait_for(system_clock) is considered to be invalid. A static assert should probably be generated here.

I think we need four uniquely named APIs for each of these scenarios or four clearly named template function specialisations for both POSIX and Windows. All timed waits anywhere in Boost.Thread go through one of those four functions. Each API needs to take a parameter, or be named differently, to indicate whether it is interruptible or not.

All the legacy timed wait functions need to be converted into one of these four wait APIs too. Personally speaking I think it is time to mandatory Chrono instead of making it optional.

Obviously this is quite a bit of work. But it serves no one well if someone presses suspend on their computer and when they wake it up all their Boost applications have hanged :(

I was originally given a few paid hours to look into the condvar problem on Windows. I may be able to ask for more hours to fix more widely, then I can help you out more than just on Windows if you need me for that. In particular I think we need some extra tests which verify all this works correctly.

Niall

viboes — Sun, 25 Jan 2015 16:58:09 GMT

Point 4 has no sens. duration is not dependent on any clock.

I don't see why we would need the 3 API. Each platform could provide only one of them. E.g. pthread provides only wait having as parameter the an absolute time.

I'm not for breaking the user interface. Users that didn't linked with Chrono, shouldn't do it no if they don't use it.

I agree for fixing all this stuff if the result is alway backward compatible (Or having the consensus that Boost.Thread can break user code on the Boost ML).

If you want to work on a clean Boost.Thread implementation I suggest you to contribute on a new completely separated version V5 for C++11 compilers as I plan to do. Are you interested?

Agreed for the test.

Niall Douglas — Sun, 25 Jan 2015 19:34:49 GMT

Replying to viboes:

I don't see why we would need the 3 API. Each platform could provide only one of them. E.g. pthread provides only wait having as parameter the an absolute time.

They would only be internal API. Public code cannot see them.

My reason is because if you unify all the timed wait logic into a single location for all of Thread instead of each threading primitive doing its own thing, it makes it much easier to debug, coordinate and extend into the future. For example, if you implement a timed_wait_for_any(future) it is using identical timed wait code to timed_mutex.

Sure, on POSIX all three functions convert into pthread absolute timed waits. But it is an API abstraction to help maintenance, that's all.

I agree for fixing all this stuff if the result is alway backward compatible (Or having the consensus that Boost.Thread can break user code on the Boost ML).

The API I proposed is completely internal.

If you want to work on a clean Boost.Thread implementation I suggest you to contribute on a new completely separated version V5 for C++11 compilers as I plan to do. Are you interested?

I'll reply to that by private email.

Niall

type changed

viboes — Mon, 02 Feb 2015 05:51:49 GMT

type Bugs → Tasks

After spending some time I don't see any bug here. Of course the code can be rewritten to be more clear.

Moving to a Task until a bug is clearly identified.

viboes — Tue, 14 Nov 2017 20:55:59 GMT

Note that we are working on a branch that pretend to fix all time related issues.