libc++ tests: wait_until.pass test sporadically fails (bug 21998)

[iid_iunknown]

Summary

Hello Howard,

While running the libc++ tests on our ARM boards, we encounter sporadic failures of the two tests:
test/std/thread/futures/futures.shared_future/wait_until.pass.cpp
test/std/thread/futures/futures.unique_future/wait_until.pass.cpp

The worker thread might not finish yet when the main thread checks its result.
I filed the bug 21998 for this case: http://llvm.org/bugs/show_bug.cgi?id=21998

Would you be able to review this please?
Thank you.
Oleg

[EricWF]

Adding the appropriate reviewers and putting in my two cents.

test/std/thread/futures/futures.shared_future/wait_until.pass.cpp

86–88

While this patch may fix the race condition that you and others have seen I think it introduces the possibility that the test could hang.

Isn't in possible (although very unlikely) that func1 will execute lines 48-53 and set thread_state = Exiting before we enter line 88?
It seems we depend on std::this_thread::sleep_for(ms(500)) to prevent this from happening. Please let me know if I am mistaken.

With our change the race condition now takes 500ms as opposed to 100ms (as mentioned in PR21998). If we are going to depend on the call to std::this_thread::sleep_for at all then why not depend on it entirely and increase the amount of time we sleep for? At least that way we prevent these tests from becoming dependent on std::mutex, std::unique_lock and std::condition_variable.

I'm also a little uncomfortable using explicit locking when testing <future>. My concern is that it might hide race conditions that would otherwise be detectable from sporadic test failures or instrumentation like thread sanitizer. However this concern could be entirely unfounded.

[iid_iunknown]

Patch updated taking into account the review remarks.

[jroelofs]

I'd add some asserts in the main thread to call out what the worker thread state is supposed to be in a few places (to improve readability). Other than that, LGTM.

[EricWF]

LGTM after the inline comment in answered. Do you need somebody to commit this for you?

test/std/thread/futures/futures.shared_future/wait_until.pass.cpp

31

Why use a relaxed and not strict memory ordering? If we use strict ordering (the default) then we can just use the assignment operators to write to thread_state and we can remove this function.

test/std/thread/futures/futures.shared_future/wait_until.pass.cpp

 
 // class shared_future<R>
 
 // template <class Clock, class Duration>
 //   future_status
 //   wait_until(const chrono::time_point<Clock, Duration>& abs_time) const;
 
 #include <future>
+#include <atomic>
 #include <cassert>
 
+enum class WorkerThreadState { Uninitialized, AllowedToRun, Exiting };
 typedef std::chrono::milliseconds ms;
 
+std::atomic<WorkerThreadState> thread_state(WorkerThreadState::Uninitialized);
+
+void set_worker_thread_state(WorkerThreadState state)
+{
+    thread_state.store(state, std::memory_order_relaxed);
+}
+
+void wait_for_worker_thread_state(WorkerThreadState state)
+{
+    while (thread_state.load(std::memory_order_relaxed) != state);
+}
+
 void func1(std::promise<int> p)
 {
-    std::this_thread::sleep_for(ms(500));
+    wait_for_worker_thread_state(WorkerThreadState::AllowedToRun);
     p.set_value(3);
+    set_worker_thread_state(WorkerThreadState::Exiting);
 }
 
 int j = 0;
 
 void func3(std::promise<int&> p)
 {
-    std::this_thread::sleep_for(ms(500));
+    wait_for_worker_thread_state(WorkerThreadState::AllowedToRun);
     j = 5;
     p.set_value(j);
+    set_worker_thread_state(WorkerThreadState::Exiting);
 }
 
 void func5(std::promise<void> p)
 {
-    std::this_thread::sleep_for(ms(500));
+    wait_for_worker_thread_state(WorkerThreadState::AllowedToRun);
     p.set_value();
+    set_worker_thread_state(WorkerThreadState::Exiting);
 }
 
 int main()
 {
     typedef std::chrono::high_resolution_clock Clock;
     {
         typedef int T;
         std::promise<T> p;
         std::shared_future<T> f = p.get_future();
         std::thread(func1, std::move(p)).detach();
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::timeout);
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::timeout);
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::ready);
+
+        // allow the worker thread to produce the result and wait until the worker is done
+        set_worker_thread_state(WorkerThreadState::AllowedToRun);
+        wait_for_worker_thread_state(WorkerThreadState::Exiting);
+
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::ready);
         assert(f.valid());
         Clock::time_point t0 = Clock::now();
         f.wait();
         Clock::time_point t1 = Clock::now();
         assert(f.valid());
         assert(t1-t0 < ms(5));
     }
     {
         typedef int& T;
         std::promise<T> p;
         std::shared_future<T> f = p.get_future();
         std::thread(func3, std::move(p)).detach();
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::timeout);
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::timeout);
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::ready);
+
+        // allow the worker thread to produce the result and wait until the worker is done
+        set_worker_thread_state(WorkerThreadState::AllowedToRun);
+        wait_for_worker_thread_state(WorkerThreadState::Exiting);
+
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::ready);
         assert(f.valid());
         Clock::time_point t0 = Clock::now();
         f.wait();
         Clock::time_point t1 = Clock::now();
         assert(f.valid());
         assert(t1-t0 < ms(5));
     }
     {
         typedef void T;
         std::promise<T> p;
         std::shared_future<T> f = p.get_future();
         std::thread(func5, std::move(p)).detach();
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::timeout);
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::timeout);
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::ready);
+
+        // allow the worker thread to produce the result and wait until the worker is done
+        set_worker_thread_state(WorkerThreadState::AllowedToRun);
+        wait_for_worker_thread_state(WorkerThreadState::Exiting);
+
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::ready);
         assert(f.valid());
         Clock::time_point t0 = Clock::now();
         f.wait();
         Clock::time_point t1 = Clock::now();
         assert(f.valid());
         assert(t1-t0 < ms(5));
     }
 }

test/std/thread/futures/futures.unique_future/wait_until.pass.cpp

 
 // class future<R>
 
 // template <class Clock, class Duration>
 //   future_status
 //   wait_until(const chrono::time_point<Clock, Duration>& abs_time) const;
 
 #include <future>
+#include <atomic>
 #include <cassert>
 
+enum class WorkerThreadState { Uninitialized, AllowedToRun, Exiting };
 typedef std::chrono::milliseconds ms;
 
+std::atomic<WorkerThreadState> thread_state(WorkerThreadState::Uninitialized);
+
+void set_worker_thread_state(WorkerThreadState state)
+{
+    thread_state.store(state, std::memory_order_relaxed);
+}
+
+void wait_for_worker_thread_state(WorkerThreadState state)
+{
+    while (thread_state.load(std::memory_order_relaxed) != state);
+}
+
 void func1(std::promise<int> p)
 {
-    std::this_thread::sleep_for(ms(500));
+    wait_for_worker_thread_state(WorkerThreadState::AllowedToRun);
     p.set_value(3);
+    set_worker_thread_state(WorkerThreadState::Exiting);
 }
 
 int j = 0;
 
 void func3(std::promise<int&> p)
 {
-    std::this_thread::sleep_for(ms(500));
+    wait_for_worker_thread_state(WorkerThreadState::AllowedToRun);
     j = 5;
     p.set_value(j);
+    set_worker_thread_state(WorkerThreadState::Exiting);
 }
 
 void func5(std::promise<void> p)
 {
-    std::this_thread::sleep_for(ms(500));
+    wait_for_worker_thread_state(WorkerThreadState::AllowedToRun);
     p.set_value();
+    set_worker_thread_state(WorkerThreadState::Exiting);
 }
 
 int main()
 {
     typedef std::chrono::high_resolution_clock Clock;
     {
         typedef int T;
         std::promise<T> p;
         std::future<T> f = p.get_future();
         std::thread(func1, std::move(p)).detach();
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::timeout);
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::timeout);
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::ready);
+
+        // allow the worker thread to produce the result and wait until the worker is done
+        set_worker_thread_state(WorkerThreadState::AllowedToRun);
+        wait_for_worker_thread_state(WorkerThreadState::Exiting);
+
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::ready);
         assert(f.valid());
         Clock::time_point t0 = Clock::now();
         f.wait();
         Clock::time_point t1 = Clock::now();
         assert(f.valid());
         assert(t1-t0 < ms(5));
     }
     {
         typedef int& T;
         std::promise<T> p;
         std::future<T> f = p.get_future();
         std::thread(func3, std::move(p)).detach();
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::timeout);
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::timeout);
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::ready);
+
+        // allow the worker thread to produce the result and wait until the worker is done
+        set_worker_thread_state(WorkerThreadState::AllowedToRun);
+        wait_for_worker_thread_state(WorkerThreadState::Exiting);
+
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::ready);
         assert(f.valid());
         Clock::time_point t0 = Clock::now();
         f.wait();
         Clock::time_point t1 = Clock::now();
         assert(f.valid());
         assert(t1-t0 < ms(5));
     }
     {
         typedef void T;
         std::promise<T> p;
         std::future<T> f = p.get_future();
         std::thread(func5, std::move(p)).detach();
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::timeout);
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::timeout);
         assert(f.valid());
-        assert(f.wait_until(Clock::now() + ms(300)) == std::future_status::ready);
+
+        // allow the worker thread to produce the result and wait until the worker is done
+        set_worker_thread_state(WorkerThreadState::AllowedToRun);
+        wait_for_worker_thread_state(WorkerThreadState::Exiting);
+
+        assert(f.wait_until(Clock::now() + ms(10)) == std::future_status::ready);
         assert(f.valid());
         Clock::time_point t0 = Clock::now();
         f.wait();
         Clock::time_point t1 = Clock::now();
         assert(f.valid());
         assert(t1-t0 < ms(5));
     }
 }