Timed wait points inconsistently convert relative to absolute waits

[Niall Douglas]

Possibly related: #6787

Stepping through the Boost.Thread win32 implementation, interruptible_wait() and non_interruptible_wait() are capable of accepting both relative and absolute timeouts. However condition_variable and thread join are always implemented as absolute timeouts, while thread sleep is always implemented as relative timeouts. I also see that timed mutex always converts absolute to relative too on win32.

These all should be fixed to correctly use either relative or absolute timeouts as requested by the user, firstly at the upper layers where necessary. Then all waits on Windows always need to go via interruptible_wait() or non_interruptible_wait() as appropriate as these implement the proper calls on Windows for absolute deadline timers when absolute timeouts are requested.

[viboes]

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

[viboes]

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]

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]

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;
        }

[viboes]

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]

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

1. 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).

2. 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).

3. 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).

4. 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]

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]

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

[viboes]

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]

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