[3/3] ntdll: Implement RtlDeleteTimer for kernel32's DeleteTimerQueueTimer.

Dan Hipschman dsh at linux.ucla.edu
Thu Jul 24 18:27:54 CDT 2008


This adds quite a few more tests for deleting since it turns out Windows'
behavior was more complicated than I thought.  I initially believed that
if the return was asynchronous (the completion event was not
INVALID_HANDLE_VALUE), that the return value would always be FALSE with an
error of ERROR_IO_PENDING.  It turns out that Windows always returns FALSE
with the pending error if the event is NULL, but if the event is valid it
will return TRUE if the timer is finished (no pending callbacks).

I've added tests for these but the timing is a bit tricky.  I didn't want
to end up sleeping/waiting for long periods of time, but needed to be
reasonably sure the callbacks would/wouldn't be finished by the time I
delete the timer.  I ran the tests a few thousand times and four of the
new tests added here fail a little less than 1% of the time.  I can bump
up the delays which should make failure happen less, but will make the
tests slower.

The four tests are these three:

    SetLastError(0xdeadbeef);
    ret = pDeleteTimerQueueTimer(q, t3, et1);
    ok(ret, "DeleteTimerQueueTimer\n");
    ok(GetLastError() == 0xdeadbeef, "DeleteTimerQueueTimer\n");
    ok(WaitForSingleObject(et1, 250) == WAIT_OBJECT_0,
       "Timer destruction event not triggered\n");

and this one:

    ok(WaitForSingleObject(et2, 1000) == WAIT_OBJECT_0,
       "Timer destruction event not triggered\n");

As for the implementation, I had a choice of always creating an event if
the completion event is INVALID_HANDLE_VALUE (which is what I do in this
patch), or bypassing this if the runcount is zero since then we don't need
to wait.  The second option requires creating an event inside the critsec,
so I didn't know which was worse: making a server round trip always,
outside the cs; or once in a while inside the cs.

Sorry for such long notes on a fairly simple patch, but I'd like to know
what the preferred solution to some of these problems are.  (If the patch
is fine the way it is, that's good too.)  ;)

Thanks.

---
 dlls/kernel32/sync.c       |   11 +++--
 dlls/kernel32/tests/sync.c |   89 +++++++++++++++++++++++++++++++++++++++-----
 dlls/ntdll/ntdll.spec      |    2 +-
 dlls/ntdll/threadpool.c    |   52 +++++++++++++++++++++++++
 include/winternl.h         |    1 +
 5 files changed, 139 insertions(+), 16 deletions(-)

diff --git a/dlls/kernel32/sync.c b/dlls/kernel32/sync.c
index 8bd4243..5291904 100644
--- a/dlls/kernel32/sync.c
+++ b/dlls/kernel32/sync.c
@@ -1131,15 +1131,16 @@ BOOL WINAPI ChangeTimerQueueTimer( HANDLE TimerQueue, HANDLE Timer,
  *
  * RETURNS
  *   nonzero on success or zero on failure
- *
- * BUGS
- *   Unimplemented
  */
 BOOL WINAPI DeleteTimerQueueTimer( HANDLE TimerQueue, HANDLE Timer,
                                    HANDLE CompletionEvent )
 {
-    FIXME("stub\n");
-    SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
+    NTSTATUS status = RtlDeleteTimer(TimerQueue, Timer, CompletionEvent);
+    if (status != STATUS_SUCCESS)
+    {
+        SetLastError( RtlNtStatusToDosError(status) );
+        return FALSE;
+    }
     return TRUE;
 }
 
diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c
index 19fc8e2..c7a949c 100644
--- a/dlls/kernel32/tests/sync.c
+++ b/dlls/kernel32/tests/sync.c
@@ -566,11 +566,8 @@ static void CALLBACK timer_queue_cb2(PVOID p, BOOLEAN timedOut)
         /* Note, XP SP2 does *not* do any deadlock checking, so passing
            INVALID_HANDLE_VALUE here will just hang.  */
         ret = pDeleteTimerQueueTimer(d->q, d->t, NULL);
-        todo_wine
-        {
         ok(!ret, "DeleteTimerQueueTimer\n");
         ok(GetLastError() == ERROR_IO_PENDING, "DeleteTimerQueueTimer\n");
-        }
     }
 }
 
@@ -604,12 +601,20 @@ static void CALLBACK timer_queue_cb4(PVOID p, BOOLEAN timedOut)
     }
 }
 
+static void CALLBACK timer_queue_cb5(PVOID p, BOOLEAN timedOut)
+{
+    DWORD delay = (DWORD) p;
+    ok(timedOut, "Timer callbacks should always time out\n");
+    if (delay)
+        Sleep(delay);
+}
+
 static void test_timer_queue(void)
 {
     HANDLE q, t1, t2, t3, t4, t5;
     int n1, n2, n3, n4, n5;
     struct timer_queue_data1 d2, d3, d4;
-    HANDLE e;
+    HANDLE e, et1, et2;
     BOOL ret;
 
     if (!pChangeTimerQueueTimer || !pCreateTimerQueue || !pCreateTimerQueueTimer
@@ -675,6 +680,14 @@ static void test_timer_queue(void)
     /* Give them a chance to do some work.  */
     Sleep(500);
 
+    /* Test deleting a once-only timer.  */
+    ret = pDeleteTimerQueueTimer(q, t1, INVALID_HANDLE_VALUE);
+    ok(ret, "DeleteTimerQueueTimer\n");
+
+    /* A periodic timer.  */
+    ret = pDeleteTimerQueueTimer(q, t2, INVALID_HANDLE_VALUE);
+    ok(ret, "DeleteTimerQueueTimer\n");
+
     ret = pDeleteTimerQueueEx(q, INVALID_HANDLE_VALUE);
     ok(ret, "DeleteTimerQueueEx\n");
     ok(n1 == 1, "Timer callback 1\n");
@@ -682,9 +695,11 @@ static void test_timer_queue(void)
     ok(n4 == 0, "Timer callback 4\n");
     ok(n5 == 1, "Timer callback 5\n");
 
-    /* Test synchronous deletion of the queue with event trigger. */
+    /* Test synchronous deletion of the timer/queue with event trigger. */
     e = CreateEvent(NULL, TRUE, FALSE, NULL);
-    if (!e)
+    et1 = CreateEvent(NULL, TRUE, FALSE, NULL);
+    et2 = CreateEvent(NULL, TRUE, FALSE, NULL);
+    if (!e || !et1 || !et2)
     {
         skip("Failed to create timer queue descruction event\n");
         return;
@@ -693,12 +708,69 @@ static void test_timer_queue(void)
     q = pCreateTimerQueue();
     ok(q != NULL, "CreateTimerQueue\n");
 
+    /* Run once and finish quickly (should be done when we delete it).  */
+    t1 = NULL;
+    ret = pCreateTimerQueueTimer(&t1, q, timer_queue_cb5, (PVOID) 0, 0,
+                                 0, 0);
+    ok(ret, "CreateTimerQueueTimer\n");
+    ok(t1 != NULL, "CreateTimerQueueTimer\n");
+
+    /* Run once and finish slowly (shouldn't be done when we delete it).  */
+    t2 = NULL;
+    ret = pCreateTimerQueueTimer(&t2, q, timer_queue_cb5, (PVOID) 1000, 0,
+                                 0, 0);
+    ok(ret, "CreateTimerQueueTimer\n");
+    ok(t2 != NULL, "CreateTimerQueueTimer\n");
+
+    /* Run once and finish quickly (should be done when we delete it).  */
+    t3 = NULL;
+    ret = pCreateTimerQueueTimer(&t3, q, timer_queue_cb5, (PVOID) 0, 0,
+                                 0, 0);
+    ok(ret, "CreateTimerQueueTimer\n");
+    ok(t3 != NULL, "CreateTimerQueueTimer\n");
+
+    /* Run once and finish slowly (shouldn't be done when we delete it).  */
+    t4 = NULL;
+    ret = pCreateTimerQueueTimer(&t4, q, timer_queue_cb5, (PVOID) 1000, 0,
+                                 0, 0);
+    ok(ret, "CreateTimerQueueTimer\n");
+    ok(t4 != NULL, "CreateTimerQueueTimer\n");
+
+    /* Give them a chance to start.  */
+    Sleep(400);
+
+    /* DeleteTimerQueueTimer always returns PENDING with a NULL event,
+       even if the timer is finished.  */
+    SetLastError(0xdeadbeef);
+    ret = pDeleteTimerQueueTimer(q, t1, NULL);
+    ok(!ret, "DeleteTimerQueueTimer\n");
+    ok(GetLastError() == ERROR_IO_PENDING, "DeleteTimerQueueTimer\n");
+
+    SetLastError(0xdeadbeef);
+    ret = pDeleteTimerQueueTimer(q, t2, NULL);
+    ok(!ret, "DeleteTimerQueueTimer\n");
+    ok(GetLastError() == ERROR_IO_PENDING, "DeleteTimerQueueTimer\n");
+
+    SetLastError(0xdeadbeef);
+    ret = pDeleteTimerQueueTimer(q, t3, et1);
+    ok(ret, "DeleteTimerQueueTimer\n");
+    ok(GetLastError() == 0xdeadbeef, "DeleteTimerQueueTimer\n");
+    ok(WaitForSingleObject(et1, 250) == WAIT_OBJECT_0,
+       "Timer destruction event not triggered\n");
+
+    SetLastError(0xdeadbeef);
+    ret = pDeleteTimerQueueTimer(q, t4, et2);
+    ok(!ret, "DeleteTimerQueueTimer\n");
+    ok(GetLastError() == ERROR_IO_PENDING, "DeleteTimerQueueTimer\n");
+    ok(WaitForSingleObject(et2, 1000) == WAIT_OBJECT_0,
+       "Timer destruction event not triggered\n");
+
     SetLastError(0xdeadbeef);
     ret = pDeleteTimerQueueEx(q, e);
     ok(!ret, "DeleteTimerQueueEx\n");
     ok(GetLastError() == ERROR_IO_PENDING, "DeleteTimerQueueEx\n");
     ok(WaitForSingleObject(e, 250) == WAIT_OBJECT_0,
-       "Timer destruction event not triggered\n");
+       "Queue destruction event not triggered\n");
     CloseHandle(e);
 
     /* Test deleting/changing a timer in execution.  */
@@ -749,10 +821,7 @@ static void test_timer_queue(void)
     ret = pDeleteTimerQueueEx(q, INVALID_HANDLE_VALUE);
     ok(ret, "DeleteTimerQueueEx\n");
     ok(n1 == 1, "ChangeTimerQueueTimer\n");
-    todo_wine
-    {
     ok(d2.num_calls == d2.max_calls, "DeleteTimerQueueTimer\n");
-    }
     ok(d3.num_calls == d3.max_calls, "ChangeTimerQueueTimer\n");
     ok(d4.num_calls == 1, "Timer flagged for deletion incorrectly\n");
 }
diff --git a/dlls/ntdll/ntdll.spec b/dlls/ntdll/ntdll.spec
index 4d200eb..e18f3a3 100644
--- a/dlls/ntdll/ntdll.spec
+++ b/dlls/ntdll/ntdll.spec
@@ -518,7 +518,7 @@
 @ stdcall RtlDeleteRegistryValue(long ptr ptr)
 @ stdcall RtlDeleteResource(ptr)
 @ stdcall RtlDeleteSecurityObject(ptr)
-# @ stub RtlDeleteTimer
+@ stdcall RtlDeleteTimer(ptr ptr ptr)
 # @ stub RtlDeleteTimerQueue
 @ stdcall RtlDeleteTimerQueueEx(ptr ptr)
 @ stdcall RtlDeregisterWait(ptr)
diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c
index f6e6c1e..a60adfa 100644
--- a/dlls/ntdll/threadpool.c
+++ b/dlls/ntdll/threadpool.c
@@ -545,6 +545,7 @@ struct queue_timer
     ULONG flags;
     ULONGLONG expire;
     BOOL destroy;      /* timer should be deleted; once set, never unset */
+    HANDLE event;      /* removal event */
 };
 
 struct timer_queue
@@ -569,6 +570,8 @@ static void queue_remove_timer(struct queue_timer *t)
     assert(t->destroy);
 
     list_remove(&t->entry);
+    if (t->event)
+        NtSetEvent(t->event, NULL);
     RtlFreeHeap(GetProcessHeap(), 0, t);
 
     if (q->quit && list_count(&q->timers) == 0)
@@ -891,6 +894,7 @@ NTSTATUS WINAPI RtlCreateTimer(PHANDLE NewTimer, HANDLE TimerQueue,
     t->period = Period;
     t->flags = Flags;
     t->destroy = FALSE;
+    t->event = NULL;
 
     status = STATUS_SUCCESS;
     RtlEnterCriticalSection(&q->cs);
@@ -943,3 +947,51 @@ NTSTATUS WINAPI RtlUpdateTimer(HANDLE TimerQueue, HANDLE Timer,
 
     return STATUS_SUCCESS;
 }
+
+/***********************************************************************
+ *              RtlDeleteTimer   (NTDLL.@)
+ *
+ * Cancels a timer-queue timer.
+ *
+ * PARAMS
+ *  TimerQueue      [I] The queue that holds the timer.
+ *  Timer           [I] The timer to update.
+ *  CompletionEvent [I] If NULL, return immediately.  If INVALID_HANDLE_VALUE,
+ *                      wait until the timer is finished firing all pending
+ *                      callbacks before returning.  Otherwise, return
+ *                      immediately and set the timer is done.
+ *
+ * RETURNS
+ *  Success: STATUS_SUCCESS if the timer is done, STATUS_PENDING if not,
+             or if the completion event is NULL.
+ *  Failure: Any NTSTATUS code.
+ */
+NTSTATUS WINAPI RtlDeleteTimer(HANDLE TimerQueue, HANDLE Timer,
+                               HANDLE CompletionEvent)
+{
+    struct timer_queue *q = TimerQueue;
+    struct queue_timer *t = Timer;
+    NTSTATUS status = STATUS_PENDING;
+    HANDLE event = NULL;
+
+    if (CompletionEvent == INVALID_HANDLE_VALUE)
+        status = NtCreateEvent(&event, EVENT_ALL_ACCESS, NULL, FALSE, FALSE);
+    else if (CompletionEvent)
+        event = CompletionEvent;
+
+    RtlEnterCriticalSection(&q->cs);
+    t->event = event;
+    if (t->runcount == 0 && event)
+        status = STATUS_SUCCESS;
+    queue_destroy_timer(t);
+    RtlLeaveCriticalSection(&q->cs);
+
+    if (CompletionEvent == INVALID_HANDLE_VALUE && event)
+    {
+        if (status == STATUS_PENDING)
+            NtWaitForSingleObject(event, FALSE, NULL);
+        NtClose(event);
+    }
+
+    return status;
+}
diff --git a/include/winternl.h b/include/winternl.h
index cb9a6f7..2dc1fc2 100644
--- a/include/winternl.h
+++ b/include/winternl.h
@@ -2129,6 +2129,7 @@ NTSYSAPI NTSTATUS  WINAPI RtlDeleteCriticalSection(RTL_CRITICAL_SECTION *);
 NTSYSAPI NTSTATUS  WINAPI RtlDeleteRegistryValue(ULONG, PCWSTR, PCWSTR);
 NTSYSAPI void      WINAPI RtlDeleteResource(LPRTL_RWLOCK);
 NTSYSAPI NTSTATUS  WINAPI RtlDeleteSecurityObject(PSECURITY_DESCRIPTOR*);
+NTSYSAPI NTSTATUS  WINAPI RtlDeleteTimer(HANDLE, HANDLE, HANDLE);
 NTSYSAPI NTSTATUS  WINAPI RtlDeleteTimerQueueEx(HANDLE, HANDLE);
 NTSYSAPI PRTL_USER_PROCESS_PARAMETERS WINAPI RtlDeNormalizeProcessParams(RTL_USER_PROCESS_PARAMETERS*);
 NTSYSAPI NTSTATUS  WINAPI RtlDeregisterWait(HANDLE);



More information about the wine-patches mailing list