[PATCH 0/1] MR10177: ntdll: Fix threadpool timer behavior on system time change.

[Piotr Caban (＠piotr)]

Threadpool timers are not handling system clock changes correctly. Observed while trying to start delayed-start service on DST change (service started after 1h02m instead of 2m).

These patch changes threadpool timers to use NtCreateTimers internally. It matches with native implementation as documented here: https://github.com/SafeBreach-Labs/PoolParty. While it moves implementation closer to what native does I didn't work on making it binary compatible.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10177

[PATCH 1/1] ntdll: Fix threadpool timer behavior on system time change.

[Piotr Caban]

From: Piotr Caban <[e-mail]>

---
 dlls/kernel32/tests/timer.c |  73 ++++++++-
 dlls/ntdll/threadpool.c     | 284 ++++++++++++++++++++++++------------
 2 files changed, 260 insertions(+), 97 deletions(-)

diff --git a/dlls/kernel32/tests/timer.c b/dlls/kernel32/tests/timer.c
index 47cfd11062e..2106c5b35c2 100644
--- a/dlls/kernel32/tests/timer.c
+++ b/dlls/kernel32/tests/timer.c
@@ -203,9 +203,78 @@ static DWORD WINAPI thread_SetTimer(void *arg)
     return 0;
 }
 
+static void WINAPI timer_callback(PTP_CALLBACK_INSTANCE inst, void* ctx, PTP_TIMER timer)
+{
+    HANDLE event = ctx;
+    SetEvent(event);
+}
+
+static DWORD WINAPI thread_SetThreadpoolTimer_rel(void *arg)
+{
+    LARGE_INTEGER li;
+    TP_TIMER *timer;
+    HANDLE event;
+    FILETIME ft;
+    DWORD t, r;
+
+    event = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(event != NULL, "CreateEvent failed\n");
+    timer = CreateThreadpoolTimer(timer_callback, event, NULL);
+    ok(timer != NULL, "CreateThreadpoolTimer failed\n");
+
+    li.QuadPart = -3 * TICKSPERSEC;
+    ft.dwHighDateTime = li.HighPart;
+    ft.dwLowDateTime = li.LowPart;
+    t = GetTickCount();
+    SetThreadpoolTimer(timer, &ft, 0, 0);
+
+    r = WaitForSingleObject(event, INFINITE);
+    ok(r == WAIT_OBJECT_0, "WaitForSingleObject returned %ld\n", r);
+    t = GetTickCount() - t;
+    ok(t > 2000, "t = %ld\n", t);
+
+    WaitForThreadpoolTimerCallbacks(timer, FALSE);
+    CloseThreadpoolTimer(timer);
+    CloseHandle(event);
+    return 0;
+}
+
+static DWORD WINAPI thread_SetThreadpoolTimer_abs(void *arg)
+{
+    LARGE_INTEGER li;
+    TP_TIMER *timer;
+    HANDLE event;
+    FILETIME ft;
+    DWORD t, r;
+
+    event = CreateEventW(NULL, FALSE, FALSE, NULL);
+    ok(event != NULL, "CreateEvent failed\n");
+    timer = CreateThreadpoolTimer(timer_callback, event, NULL);
+    ok(timer != NULL, "CreateThreadpoolTimer failed\n");
+
+    GetSystemTimeAsFileTime(&ft);
+    li.HighPart = ft.dwHighDateTime;
+    li.LowPart = ft.dwLowDateTime;
+    li.QuadPart += 3 * TICKSPERSEC;
+    ft.dwHighDateTime = li.HighPart;
+    ft.dwLowDateTime = li.LowPart;
+    t = GetTickCount();
+    SetThreadpoolTimer(timer, &ft, 0, 0);
+
+    r = WaitForSingleObject(event, INFINITE);
+    ok(r == WAIT_OBJECT_0, "WaitForSingleObject returned %ld\n", r);
+    t = GetTickCount() - t;
+    ok(t < 2000, "t = %ld\n", t);
+
+    WaitForThreadpoolTimerCallbacks(timer, FALSE);
+    CloseThreadpoolTimer(timer);
+    CloseHandle(event);
+    return 0;
+}
+
 static void test_timeouts(void)
 {
-    HANDLE threads[7];
+    HANDLE threads[9];
     DWORD i;
 
     if (!adjust_system_time(1))
@@ -222,6 +291,8 @@ static void test_timeouts(void)
     threads[4] = CreateThread(NULL, 0, thread_WaitableTimer_abs, NULL, 0, NULL);
     threads[5] = CreateThread(NULL, 0, thread_WaitableTimer_period, NULL, 0, NULL);
     threads[6] = CreateThread(NULL, 0, thread_SetTimer, NULL, 0, NULL);
+    threads[7] = CreateThread(NULL, 0, thread_SetThreadpoolTimer_rel, NULL, 0, NULL);
+    threads[8] = CreateThread(NULL, 0, thread_SetThreadpoolTimer_abs, NULL, 0, NULL);
     for(i=0; i<ARRAY_SIZE(threads); i++)
         ok(threads[i] != NULL, "CreateThread failed\n");
 
diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c
index 64a25694467..0aacfa6449f 100644
--- a/dlls/ntdll/threadpool.c
+++ b/dlls/ntdll/threadpool.c
@@ -240,21 +240,29 @@ struct threadpool_group
 /* global timerqueue object */
 static RTL_CRITICAL_SECTION_DEBUG timerqueue_debug;
 
+enum
+{
+    ABS_TIMER,
+    REL_TIMER
+};
+
 static struct
 {
     CRITICAL_SECTION        cs;
     LONG                    objcount;
     BOOL                    thread_running;
-    struct list             pending_timers;
-    RTL_CONDITION_VARIABLE  update_event;
+    HANDLE                  timers[2];
+    struct list             abs_timers;
+    struct list             rel_timers;
 }
 timerqueue =
 {
     { &timerqueue_debug, -1, 0, 0, 0, 0 },      /* cs */
     0,                                          /* objcount */
     FALSE,                                      /* thread_running */
-    LIST_INIT( timerqueue.pending_timers ),     /* pending_timers */
-    RTL_CONDITION_VARIABLE_INIT                 /* update_event */
+    { 0, 0 },                                   /* timers */
+    LIST_INIT( timerqueue.abs_timers ),         /* abs_timers */
+    LIST_INIT( timerqueue.rel_timers )          /* rel_timers */
 };
 
 static RTL_CRITICAL_SECTION_DEBUG timerqueue_debug =
@@ -1052,90 +1060,140 @@ NTSTATUS WINAPI RtlDeleteTimer(HANDLE TimerQueue, HANDLE Timer,
 }
 
 /***********************************************************************
- *           timerqueue_thread_proc    (internal)
+ *           submit_expired_timers     (internal)
+ *
+ * timerqueue.cs held by caller.
  */
-static void CALLBACK timerqueue_thread_proc( void *param )
+static void submit_expired_timers( struct list *queue, ULONGLONG queue_now, ULONGLONG rel_now )
 {
-    ULONGLONG timeout_lower, timeout_upper, new_timeout;
     struct threadpool_object *other_timer;
-    LARGE_INTEGER now, timeout;
     struct list *ptr;
 
-    TRACE( "starting timer queue thread\n" );
-    set_thread_name(L"wine_threadpool_timerqueue");
-
-    RtlEnterCriticalSection( &timerqueue.cs );
-    for (;;)
+    while ((ptr = list_head( queue )))
     {
-        NtQuerySystemTime( &now );
+        struct threadpool_object *timer = LIST_ENTRY( ptr, struct threadpool_object, u.timer.timer_entry );
+        assert( timer->type == TP_OBJECT_TYPE_TIMER );
+        assert( timer->u.timer.timer_pending );
+        if (timer->u.timer.timeout > queue_now)
+            return;
+
+        /* Queue a new callback in one of the worker threads. */
+        list_remove( &timer->u.timer.timer_entry );
+        timer->u.timer.timer_pending = FALSE;
+        tp_object_submit( timer, FALSE );
 
-        /* Check for expired timers. */
-        while ((ptr = list_head( &timerqueue.pending_timers )))
+        /* Insert the timer back into the queue, except it's marked for shutdown. */
+        if (timer->u.timer.period && !timer->shutdown)
         {
-            struct threadpool_object *timer = LIST_ENTRY( ptr, struct threadpool_object, u.timer.timer_entry );
-            assert( timer->type == TP_OBJECT_TYPE_TIMER );
-            assert( timer->u.timer.timer_pending );
-            if (timer->u.timer.timeout > now.QuadPart)
-                break;
+            /* Update timeout when moving timer to relative queue */
+            if (queue == &timerqueue.abs_timers)
+                timer->u.timer.timeout = rel_now;
 
-            /* Queue a new callback in one of the worker threads. */
-            list_remove( &timer->u.timer.timer_entry );
-            timer->u.timer.timer_pending = FALSE;
-            tp_object_submit( timer, FALSE );
+            timer->u.timer.timeout += (ULONGLONG)timer->u.timer.period * 10000;
+            if (timer->u.timer.timeout <= rel_now)
+                timer->u.timer.timeout = rel_now + 1;
 
-            /* Insert the timer back into the queue, except it's marked for shutdown. */
-            if (timer->u.timer.period && !timer->shutdown)
+            LIST_FOR_EACH_ENTRY( other_timer, &timerqueue.rel_timers,
+                    struct threadpool_object, u.timer.timer_entry )
             {
-                timer->u.timer.timeout += (ULONGLONG)timer->u.timer.period * 10000;
-                if (timer->u.timer.timeout <= now.QuadPart)
-                    timer->u.timer.timeout = now.QuadPart + 1;
-
-                LIST_FOR_EACH_ENTRY( other_timer, &timerqueue.pending_timers,
-                                     struct threadpool_object, u.timer.timer_entry )
-                {
-                    assert( other_timer->type == TP_OBJECT_TYPE_TIMER );
-                    if (timer->u.timer.timeout < other_timer->u.timer.timeout)
-                        break;
-                }
-                list_add_before( &other_timer->u.timer.timer_entry, &timer->u.timer.timer_entry );
-                timer->u.timer.timer_pending = TRUE;
+                assert( other_timer->type == TP_OBJECT_TYPE_TIMER );
+                if (timer->u.timer.timeout < other_timer->u.timer.timeout)
+                    break;
             }
+            list_add_before( &other_timer->u.timer.timer_entry, &timer->u.timer.timer_entry );
+            timer->u.timer.timer_pending = TRUE;
         }
+    }
+}
 
-        timeout_lower = timeout_upper = MAXLONGLONG;
+/***********************************************************************
+ *           get_next_timeout          (internal)
+ *
+ * timerqueue.cs held by caller.
+ */
+static ULONGLONG get_next_timeout( struct list *list )
+{
+    ULONGLONG timeout_lower, timeout_upper, new_timeout;
+    struct threadpool_object *other_timer;
 
-        /* Determine next timeout and use the window length to optimize wakeup times. */
-        LIST_FOR_EACH_ENTRY( other_timer, &timerqueue.pending_timers,
-                             struct threadpool_object, u.timer.timer_entry )
-        {
-            assert( other_timer->type == TP_OBJECT_TYPE_TIMER );
-            if (other_timer->u.timer.timeout >= timeout_upper)
-                break;
+    timeout_lower = timeout_upper = MAXLONGLONG;
 
-            timeout_lower = other_timer->u.timer.timeout;
-            new_timeout   = timeout_lower + (ULONGLONG)other_timer->u.timer.window_length * 10000;
-            if (new_timeout < timeout_upper)
-                timeout_upper = new_timeout;
-        }
+    /* Determine next timeout and use the window length to optimize wakeup times. */
+    LIST_FOR_EACH_ENTRY( other_timer, list, struct threadpool_object, u.timer.timer_entry )
+    {
+        assert( other_timer->type == TP_OBJECT_TYPE_TIMER );
+        if (other_timer->u.timer.timeout >= timeout_upper)
+            break;
 
-        /* Wait for timer update events or until the next timer expires. */
-        if (timerqueue.objcount)
-        {
-            timeout.QuadPart = timeout_lower;
-            RtlSleepConditionVariableCS( &timerqueue.update_event, &timerqueue.cs, &timeout );
-            continue;
-        }
+        timeout_lower = other_timer->u.timer.timeout;
+        new_timeout   = timeout_lower + (ULONGLONG)other_timer->u.timer.window_length * 10000;
+        if (new_timeout < timeout_upper)
+            timeout_upper = new_timeout;
+    }
+    return timeout_lower;
+}
 
+/***********************************************************************
+ *           update_timers             (internal)
+ *
+ * timerqueue.cs held by caller.
+ */
+static void update_timers( ULONGLONG rel_now )
+{
+    LARGE_INTEGER timeout;
+
+    timeout.QuadPart = get_next_timeout( &timerqueue.abs_timers );
+    NtSetTimer( timerqueue.timers[ABS_TIMER], &timeout, NULL, NULL, FALSE, 0, NULL );
+
+    if (timerqueue.objcount)
+    {
+        timeout.QuadPart = get_next_timeout( &timerqueue.rel_timers );
+        if (timeout.QuadPart > rel_now)
+            timeout.QuadPart = rel_now - timeout.QuadPart;
+        else
+            timeout.QuadPart = 0;
+    }
+    else
+    {
         /* All timers have been destroyed, if no new timers are created
          * within some amount of time, then we can shutdown this thread. */
         timeout.QuadPart = (ULONGLONG)THREADPOOL_WORKER_TIMEOUT * -10000;
-        if (RtlSleepConditionVariableCS( &timerqueue.update_event, &timerqueue.cs,
-            &timeout ) == STATUS_TIMEOUT && !timerqueue.objcount)
-        {
+    }
+    NtSetTimer( timerqueue.timers[REL_TIMER], &timeout, NULL, NULL, FALSE, 0, NULL );
+}
+
+/***********************************************************************
+ *           timerqueue_thread_proc    (internal)
+ */
+static void CALLBACK timerqueue_thread_proc( void *param )
+{
+    LARGE_INTEGER abs_now, rel_now;
+
+    TRACE( "starting timer queue thread\n" );
+    set_thread_name(L"wine_threadpool_timerqueue");
+
+    RtlEnterCriticalSection( &timerqueue.cs );
+    for (;;)
+    {
+        NtQuerySystemTime( &abs_now );
+        NtQueryPerformanceCounter( &rel_now, NULL );
+
+        submit_expired_timers( &timerqueue.abs_timers, abs_now.QuadPart, rel_now.QuadPart );
+        submit_expired_timers( &timerqueue.rel_timers, rel_now.QuadPart, rel_now.QuadPart );
+        update_timers( rel_now.QuadPart );
+
+        RtlLeaveCriticalSection( &timerqueue.cs );
+        NtWaitForMultipleObjects( ARRAY_SIZE(timerqueue.timers),
+                timerqueue.timers, WaitAny, FALSE, NULL );
+        RtlEnterCriticalSection( &timerqueue.cs );
+
+        if (!timerqueue.objcount)
             break;
-        }
     }
 
+    NtClose( timerqueue.timers[ABS_TIMER] );
+    NtClose( timerqueue.timers[REL_TIMER] );
+
     timerqueue.thread_running = FALSE;
     RtlLeaveCriticalSection( &timerqueue.cs );
 
@@ -1173,7 +1231,6 @@ static NTSTATUS tp_new_worker_thread( struct threadpool *pool )
  */
 static NTSTATUS tp_timerqueue_lock( struct threadpool_object *timer )
 {
-    NTSTATUS status = STATUS_SUCCESS;
     assert( timer->type == TP_OBJECT_TYPE_TIMER );
 
     timer->u.timer.timer_initialized    = FALSE;
@@ -1188,24 +1245,44 @@ static NTSTATUS tp_timerqueue_lock( struct threadpool_object *timer )
     /* Make sure that the timerqueue thread is running. */
     if (!timerqueue.thread_running)
     {
+        NTSTATUS status;
         HANDLE thread;
+
+        status = NtCreateTimer( &timerqueue.timers[ABS_TIMER], TIMER_ALL_ACCESS,
+                                NULL, NotificationTimer );
+        if (status != STATUS_SUCCESS)
+        {
+            RtlLeaveCriticalSection( &timerqueue.cs );
+            return status;
+        }
+
+        status = NtCreateTimer( &timerqueue.timers[REL_TIMER], TIMER_ALL_ACCESS,
+                NULL, NotificationTimer );
+        if (status != STATUS_SUCCESS)
+        {
+            NtClose( timerqueue.timers[ABS_TIMER] );
+            RtlLeaveCriticalSection( &timerqueue.cs );
+            return status;
+        }
+
         status = RtlCreateUserThread( GetCurrentProcess(), NULL, FALSE, 0, 0, 0,
                                       timerqueue_thread_proc, NULL, &thread, NULL );
-        if (status == STATUS_SUCCESS)
+        if (status != STATUS_SUCCESS)
         {
-            timerqueue.thread_running = TRUE;
-            NtClose( thread );
+            NtClose( timerqueue.timers[ABS_TIMER] );
+            NtClose( timerqueue.timers[REL_TIMER] );
+            RtlLeaveCriticalSection( &timerqueue.cs );
+            return status;
         }
+        timerqueue.thread_running = TRUE;
+        NtClose( thread );
     }
 
-    if (status == STATUS_SUCCESS)
-    {
-        timer->u.timer.timer_initialized = TRUE;
-        timerqueue.objcount++;
-    }
+    timer->u.timer.timer_initialized = TRUE;
+    timerqueue.objcount++;
 
     RtlLeaveCriticalSection( &timerqueue.cs );
-    return status;
+    return STATUS_SUCCESS;
 }
 
 /***********************************************************************
@@ -1227,11 +1304,15 @@ static void tp_timerqueue_unlock( struct threadpool_object *timer )
             timer->u.timer.timer_pending = FALSE;
         }
 
-        /* If the last timer object was destroyed, then wake up the thread. */
+        /* If the last timer object was destroyed, then update timeout. */
         if (!--timerqueue.objcount)
         {
-            assert( list_empty( &timerqueue.pending_timers ) );
-            RtlWakeAllConditionVariable( &timerqueue.update_event );
+            LARGE_INTEGER rel_now;
+
+            assert( list_empty( &timerqueue.abs_timers ) );
+            assert( list_empty( &timerqueue.rel_timers ) );
+            NtQueryPerformanceCounter( &rel_now, NULL );
+            update_timers( rel_now.QuadPart );
         }
 
         timer->u.timer.timer_initialized = FALSE;
@@ -2997,6 +3078,7 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO
 {
     struct threadpool_object *this = impl_from_TP_TIMER( timer );
     struct threadpool_object *other_timer;
+    struct list *pending_timers;
     BOOL submit_timer = FALSE;
     ULONGLONG timestamp;
 
@@ -3011,23 +3093,29 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO
      * of zero, which means that the timer is submitted immediately. */
     if (timeout)
     {
-        timestamp = timeout->QuadPart;
-        if ((LONGLONG)timestamp < 0)
+        if (timeout->QuadPart > 0)
         {
-            LARGE_INTEGER now;
-            NtQuerySystemTime( &now );
-            timestamp = now.QuadPart - timestamp;
+            timestamp = timeout->QuadPart;
+            pending_timers = &timerqueue.abs_timers;
         }
-        else if (!timestamp)
+        else if (timeout->QuadPart < 0)
         {
-            if (!period)
-                timeout = NULL;
-            else
-            {
-                LARGE_INTEGER now;
-                NtQuerySystemTime( &now );
-                timestamp = now.QuadPart + (ULONGLONG)period * 10000;
-            }
+            LARGE_INTEGER rel_now;
+            NtQueryPerformanceCounter( &rel_now, NULL );
+            timestamp = rel_now.QuadPart - timeout->QuadPart;
+            pending_timers = &timerqueue.rel_timers;
+        }
+        else if (!period)
+        {
+            timeout = NULL;
+            submit_timer = TRUE;
+        }
+        else
+        {
+            LARGE_INTEGER rel_now;
+            NtQueryPerformanceCounter( &rel_now, NULL );
+            timestamp = rel_now.QuadPart + (ULONGLONG)period * 10000;
+            pending_timers = &timerqueue.rel_timers;
             submit_timer = TRUE;
         }
     }
@@ -3046,8 +3134,8 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO
         this->u.timer.period        = period;
         this->u.timer.window_length = window_length;
 
-        LIST_FOR_EACH_ENTRY( other_timer, &timerqueue.pending_timers,
-                             struct threadpool_object, u.timer.timer_entry )
+        LIST_FOR_EACH_ENTRY( other_timer, pending_timers, struct threadpool_object,
+                             u.timer.timer_entry )
         {
             assert( other_timer->type == TP_OBJECT_TYPE_TIMER );
             if (this->u.timer.timeout < other_timer->u.timer.timeout)
@@ -3055,9 +3143,13 @@ VOID WINAPI TpSetTimer( TP_TIMER *timer, LARGE_INTEGER *timeout, LONG period, LO
         }
         list_add_before( &other_timer->u.timer.timer_entry, &this->u.timer.timer_entry );
 
-        /* Wake up the timer thread when the timeout has to be updated. */
-        if (list_head( &timerqueue.pending_timers ) == &this->u.timer.timer_entry )
-            RtlWakeAllConditionVariable( &timerqueue.update_event );
+        /* Update timeout if needed. */
+        if (list_head( pending_timers ) == &this->u.timer.timer_entry )
+        {
+            LARGE_INTEGER rel_now;
+            NtQueryPerformanceCounter( &rel_now, NULL );
+            update_timers( rel_now.QuadPart );
+        }
 
         this->u.timer.timer_pending = TRUE;
     }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/10177