Merge branch 'bugfix/pthread_cond_wait_timeout' into 'master'

pthread: Fix pthread_cond_timedwait returning early from timeout

Closes IDFGH-5119 and IDF-1055

See merge request espressif/esp-idf!13658

components/pthread/pthread_cond_var.c

@@ -128,7 +128,8 @@ int pthread_cond_timedwait(pthread_cond_t *cv, pthread_mutex_t *mut, const struc
         gettimeofday(&cur_time, NULL);
 
         abs_time.tv_sec = to->tv_sec;
-        abs_time.tv_usec = to->tv_nsec / 1000;
+        // Round up nanoseconds to the next microsecond
+        abs_time.tv_usec = (to->tv_nsec + 1000 - 1) / 1000;
 
         if (timercmp(&abs_time, &cur_time, <)) {
             /* As per the pthread spec, if the time has already
@@ -137,14 +138,27 @@ int pthread_cond_timedwait(pthread_cond_t *cv, pthread_mutex_t *mut, const struc
             timeout_msec = 0;
         } else {
             timersub(&abs_time, &cur_time, &diff_time);
-            timeout_msec = (diff_time.tv_sec * 1000) + (diff_time.tv_usec / 1000);
+            // Round up timeout microseconds to the next millisecond
+            timeout_msec = (diff_time.tv_sec * 1000) +
+                ((diff_time.tv_usec + 1000 - 1) / 1000);
         }
 
         if (timeout_msec <= 0) {
             return ETIMEDOUT;
         }
 
-        timeout_ticks = timeout_msec / portTICK_PERIOD_MS;
+        // Round up milliseconds to the next tick
+        timeout_ticks = (timeout_msec + portTICK_PERIOD_MS - 1) / portTICK_PERIOD_MS;
+
+        /* We have to add 1 more tick of delay
+
+           The reason for this is that vTaskDelay(1) will sleep until the start of the next tick,
+           which can be any amount of time up to one tick period. So if we don't add one more tick,
+           we're likely to timeout a small time (< 1 tick period) before the requested timeout.
+           If we add 1 tick then we will timeout a  small time (< 1 tick period) after the
+           requested timeout.
+         */
+        timeout_ticks += 1;
     }
 
     esp_pthread_cond_waiter_t w;

components/pthread/test/test_cxx_cond_var.cpp

@@ -4,13 +4,15 @@
 #include <chrono>
 #include <mutex>
 #include <atomic>
+#include <unistd.h>
+#include "freertos/FreeRTOS.h"
 #include "unity.h"
 
 #if __GTHREADS && __GTHREADS_CXX0X
 
-std::condition_variable cv;
-std::mutex cv_m;
-std::atomic<int> i{0};
+static std::condition_variable cv;
+static std::mutex cv_m;
+static std::atomic<int> i{0};
 
 static void waits(int idx, int timeout_ms)
 {
@@ -45,4 +47,52 @@ TEST_CASE("C++ condition_variable", "[std::condition_variable]")
 
     std::cout << "All threads joined\n";
 }
-#endif
+
+
+TEST_CASE("cxx: condition_variable can timeout", "[cxx]")
+{
+    std::condition_variable cv;
+    std::mutex mtx;
+    std::unique_lock<std::mutex> lck(mtx);
+    srand(99);
+    for (int i = 0; i < 10; ++i) {
+        usleep(rand() % 1000);
+        auto status = cv.wait_for(lck, std::chrono::milliseconds(200));
+        TEST_ASSERT_EQUAL(std::cv_status::timeout, status);
+    }
+}
+
+TEST_CASE("cxx: condition_variable timeout never before deadline", "[cxx]")
+{
+    using SysClock = std::chrono::system_clock;
+
+    std::mutex mutex;
+    std::condition_variable cond;
+    std::unique_lock<std::mutex> lock(mutex);
+
+    for (int i = 0; i < 25; ++i) {
+        auto timeout = std::chrono::milliseconds(portTICK_PERIOD_MS * (i+1));
+        auto deadline = SysClock::now() + timeout;
+
+        auto secs = std::chrono::time_point_cast<std::chrono::seconds>(deadline);
+        auto nsecs = std::chrono::duration_cast<std::chrono::nanoseconds>
+                (deadline - secs);
+        struct timespec ts = {
+                .tv_sec = static_cast<time_t>(secs.time_since_epoch().count()),
+                .tv_nsec = static_cast<long>(nsecs.count())};
+        int rc = ::pthread_cond_timedwait(cond.native_handle(),
+                                          lock.mutex()->native_handle(), &ts);
+        auto status = (rc == ETIMEDOUT) ? std::cv_status::timeout :
+                                          std::cv_status::no_timeout;
+        auto end = SysClock::now();
+        auto extra = end - deadline;
+        auto extra_us = extra / std::chrono::microseconds(1);
+        printf("timeout %lldms Extra time: %lldus, status: %s\n", timeout.count(), extra_us,
+               (status == std::cv_status::timeout) ? "timeout" : "no timeout");
+
+        // The timed wait should always return at least 1us after the timeout deadline
+        TEST_ASSERT_GREATER_THAN(0, extra_us);
+    }
+}
+
+#endif // __GTHREADS && __GTHREADS_CXX0X

components/pthread/test/test_pthread.c

@@ -12,6 +12,7 @@ static void *compute_square(void *arg)
 {
     int *num = (int *) arg;
     *num = (*num) * (*num);
+    vTaskDelay(2); // ensure the test task has time to continue execution
     pthread_exit((void *) num);
     return NULL;
 }

components/pthread/test/test_pthread_cxx.cpp

@@ -80,14 +80,16 @@ TEST_CASE("pthread C++", "[pthread]")
 
     std::thread t3(thread_main);
     std::thread t4(thread_main);
-    if (t3.joinable()) {
-        std::cout << "Join thread " << std::hex << t3.get_id() << std::endl;
-        t3.join();
-    }
-    if (t4.joinable()) {
-        std::cout << "Join thread " << std::hex << t4.get_id() << std::endl;
-        t4.join();
-    }
+    TEST_ASSERT(t3.joinable());
+    TEST_ASSERT(t4.joinable());
+    std::cout << "Join thread " << std::hex << t3.get_id() << std::endl;
+    t3.join();
+    std::cout << "Join thread " << std::hex << t4.get_id() << std::endl;
+    t4.join();
+
+    // we don't know if/when t2 has finished, so delay another 2s before
+    // deleting the common mutexes
+    std::this_thread::sleep_for(std::chrono::seconds(2));
 
     global_sp_mtx.reset(); // avoid reported leak
     global_sp_recur_mtx.reset();

docs/en/api-reference/system/pthread.rst

@@ -94,6 +94,8 @@ Condition Variables
 
 Static initializer constant ``PTHREAD_COND_INITIALIZER`` is supported.
 
+* The resolution of ``pthread_cond_timedwait()`` timeouts is the RTOS tick period (see :ref:`CONFIG_FREERTOS_HZ`). Timeouts may be delayed up to one tick period after the requested timeout.
+
 .. note:: These functions can be called from tasks created using either pthread or FreeRTOS APIs
 
 Thread-Specific Data