Bug#20705648 - max_statement_time leaks memory on windows

Bug#20705642 - max_statement_time: assertion failed: pending || thd_timer->thread_id

Description:
-------------
Both of these issues are related to deletion of timer from the
Timer-queue.

Issue in bug20705648 is, the timer object created in windows is
not deleted after the timer expiration. Hence memory leak is seen
on the windows.

Issue in bug20705642 is, the timer reset operation sets state of
timer as "not expired" when the timer is already expired and the
callback function execution of it is completed. This is resulting
in the assert condition failure.

Analysis:
-------------
In the current code, for the set timer operation new Timer-queue
timer is created. For timer cancel and destroy operations, timer
-queue timer is deleted from the Timer-queue. But when timer is
expired, assumption was timers of type "WT_EXECUTEONLYONCE" are
deleted. But these timer objects are not delete from Timer-queue
on expiration. Hence the memory leak is observed in the current
code.

The windows API to delete timer-queue timer,

    1 Fails when timer is expired or callback function is in
      execution.

    2 Succeeds when timer is *not* expired.

    3 Succeeds when timer is expired and callback function
      execution is completed.

In the current code, case 1 and 2 are handled properly but
case 3 is not handled. Because of which windows timer reset operation
returns timer state as not expired. If timer is not expired then
non zero thread_id value is expected (as notify function for timer
expiration is not called) in reset timer code. Assert condition to
check the same fails in debug build because of not handling
condition 3. In non-debug build, timer object is just set for the
reuse.

Fix:
------
To fix memory leak, if timer is expired then deleting it from the
queue on next timer set or timer destroy operations.

To fix assert issues, new timer_state member is introduced in the
my_timer for windows.

timer_state is set to "TIMER_SET" in timer set operation and
in timer_callback  to "TIMER_EXPIRED".

With timer_state, setting timer expired/not expired state in
timer reset operation is handled as below,

Timer reset operation (my_timer_cancel()):
  Windows API DeleteTimerQueueTimer is used to delete m
  the timer queue. This API,

  * Succeeds when timer is *not* expired.
      state is set to *not expired* as timer_state is TIMER_SET.

  * Succeeds when timer is expired and callback function
    execution is completed.
      state is set to *expired* as timer_state is TIMER_EXPIRED.

  *  *Fails* when timer is expired or callback function is in
     execution.
       state is set to *expired*.
       timer_state value is *not* checked in this case.

So to fix assert condition failure, "timer_state" value is
checked to set timer expired or non expired state. Now if
timer cancellation is called after timer callback execution
then "expired" state is returned.

Testing:
--------
Without patch, memory leak of around 400kb/sec is observed with
mysqlslap.exe -hlocalhost --number-of-queries=10000000000 --query="select
max_statement_time=1 sleep(rand()) ;" --iterations=1000000
--create-schema=test

With patch no memory leak or crash is observed,
\bin>time
The current time is: 11:21:03.07
Enter the new time:

\bin>tasklist /FI "IMAGENAME eq mysqld.exe"

Image Name                     PID Session Name        Session#    Mem Usage
========================= ======== ================ =========== ============
mysqld.exe                   11304 RDP-Tcp#28                 2    119,244 K

\bin>time
The current time is: 11:28:35.59
Enter the new time: .

\bin>tasklist /FI "IMAGENAME eq mysqld.exe"

Image Name                     PID Session Name        Session#    Mem Usage
========================= ======== ================ =========== ============
mysqld.exe                   11304 RDP-Tcp#28                 2    119,244 K

Author: phulakun

include/my_timer.h

@@ -27,7 +27,11 @@
 # include <sys/types.h> /* uintptr_t */
   typedef uintptr_t os_timer_t;
 #elif _WIN32
-  typedef HANDLE os_timer_t;
+  typedef struct st_os_timer
+  {
+    HANDLE timer_handle;
+    my_bool timer_state;
+  } os_timer_t;
 #endif
 
 typedef struct st_my_timer my_timer_t;

mysys/win_timers.c

@@ -21,7 +21,8 @@
 #include "my_timer.h"       /* my_timer_t */
 #include <windows.h>        /* Timer Queue and IO completion port functions */
 
-#define  TIMER_EXPIRED    1
+
+enum enum_timer_state { TIMER_SET=FALSE, TIMER_EXPIRED=TRUE };
 
 // Timer notifier thread id.
 static my_thread_handle timer_notify_thread;
@@ -46,7 +47,8 @@ timer_callback_function(PVOID timer_data, BOOLEAN timer_or_wait_fired __attribut
 {
   my_timer_t *timer= (my_timer_t *)timer_data;
   DBUG_ASSERT(timer != NULL);
-  PostQueuedCompletionStatus(io_compl_port, TIMER_EXPIRED, (ULONG_PTR)timer, 0);
+  timer->id.timer_state= TIMER_EXPIRED;
+  PostQueuedCompletionStatus(io_compl_port, 0, (ULONG_PTR)timer, 0);
 }
 
 
@@ -58,7 +60,7 @@ timer_callback_function(PVOID timer_data, BOOLEAN timer_or_wait_fired __attribut
 static void*
 timer_notify_thread_func(void *arg __attribute__((unused)))
 {
-  DWORD timer_state;
+  DWORD bytes_transferred;
   ULONG_PTR compl_key;
   LPOVERLAPPED overlapped;
   my_timer_t *timer;
@@ -68,17 +70,11 @@ timer_notify_thread_func(void *arg __attribute__((unused)))
   while(1)
   {
     // Get IO Completion status.
-    if (GetQueuedCompletionStatus(io_compl_port, &timer_state, &compl_key,
+    if (GetQueuedCompletionStatus(io_compl_port, &bytes_transferred, &compl_key,
                                   &overlapped, INFINITE) == 0)
       break;
 
     timer= (my_timer_t*)compl_key;
-
-    // Timer is cancelled.
-    if (timer->id == 0)
-      continue;
-
-    timer->id= 0;
     timer->notify_function(timer);
   }
 
@@ -102,7 +98,6 @@ static int
 delete_timer(my_timer_t *timer, int *state)
 {
   int ret_val;
-  HANDLE id= timer->id;
   int retry_count= 3;
 
   DBUG_ASSERT(timer != 0);
@@ -111,27 +106,87 @@ delete_timer(my_timer_t *timer, int *state)
   if (state != NULL)
     *state= 0;
 
-  if (id)
+  if (timer->id.timer_handle)
   {
     do {
-      ret_val= DeleteTimerQueueTimer(timer_queue, id, NULL);
+      ret_val= DeleteTimerQueueTimer(timer_queue, timer->id.timer_handle, NULL);
 
       if (ret_val != 0)
       {
-        // Timer is not signalled yet.
-        if (state != NULL)
+        /**
+          From MSDN documentation of DeleteTimerQueueTimer:
+
+            ------------------------------------------------------------------
+
+            BOOL WINAPI DeleteTimerQueueTimer(
+              _In_opt_ HANDLE TimerQueue,
+              _In_     HANDLE Timer,
+              _In_opt_ HANDLE CompletionEvent
+            );
+
+            ...
+            If there are outstanding callback functions and CompletionEvent is
+            NULL, the function will fail and set the error code to
+            ERROR_IO_PENDING. This indicates that there are outstanding callback
+            functions. Those callbacks either will execute or are in the middle
+            of executing.
+            ...
+
+            ------------------------------------------------------------------
+
+          So we are here only in 2 cases,
+             1 When timer is *not* expired yet.
+             2 When timer is expired and callback function execution is
+               completed.
+
+          So here in case 1 timer.id->timer_state is TIMER_SET and
+                  in case 2 timer.id->timer_state is TIMER_EXPIRED
+          (From MSDN documentation(pasted above), if timer callback function is
+           not yet executed or it is in the middle of execution then
+           DeleteTimerQueueTimer() fails. Hence when we are here, we are sure
+           that state is either TIMER_SET(case 1) or TIMER_EXPIRED(case 2))
+
+          Note:
+            timer.id->timer_state is set to TIMER_EXPIRED in
+            timer_callback_function(). This function is executed by the OS
+            thread on timer expiration.
+
+            On timer expiration, when callback function is in the middle of
+            execution or it is yet to be executed by OS thread the call to
+            DeleteTimerQueueTimer() fails with an error "ERROR_IO_PENDING".
+            In this case,  timer.id->timer_state is not accessed in the current
+            code (please check else if block below).
+
+            Since timer.id->timer_state is not accessed in the current code
+            while it is getting modified in timer_callback_function,
+            no synchronization mechanism used.
+
+          Setting state to 1(non-signaled) if timer_state is not set to
+          "TIMER_EXPIRED"
+        */
+        if (timer->id.timer_state != TIMER_EXPIRED && state != NULL)
           *state= 1;
 
-        timer->id= 0;
+        timer->id.timer_handle= 0;
       }
       else if (GetLastError() == ERROR_IO_PENDING)
       {
-        // Timer is signalled but callback function is not called yet.
+        /**
+          Timer is expired and timer callback function execution is not
+          yet completed.
+
+          Note: timer->id.timer_state is modified in callback function.
+                Accessing timer->id.timer_state here might result in
+                race conditions.
+                Currently we are not accessing timer->id.timer_state
+                here so not using any synchronization mechanism.
+        */
+        timer->id.timer_handle= 0;
         ret_val= 1;
       }
       else
       {
-        /*
+        /**
           Timer deletion from queue failed and there are no outstanding
           callback functions for this timer.
         */
@@ -238,7 +293,7 @@ int
 my_timer_create(my_timer_t *timer)
 {
   DBUG_ASSERT(timer_queue != 0);
-  timer->id= 0;
+  timer->id.timer_handle= 0;
   return 0;
 }
 
@@ -256,10 +311,19 @@ int
 my_timer_set(my_timer_t *timer, unsigned long time)
 {
   DBUG_ASSERT(timer != NULL);
-  DBUG_ASSERT(timer->id == 0);
   DBUG_ASSERT(timer_queue != 0);
 
-  if (CreateTimerQueueTimer(&timer->id, timer_queue,
+  /**
+    If timer set previously is expired then it will not be
+    removed from the timer queue. Removing it before creating
+    a new timer queue timer.
+  */
+  if (timer->id.timer_handle != 0)
+    my_timer_delete(timer);
+
+  timer->id.timer_state= TIMER_SET;
+
+  if (CreateTimerQueueTimer(&timer->id.timer_handle, timer_queue,
                             timer_callback_function, timer, time, 0,
                             WT_EXECUTEONLYONCE) == 0)
     return -1;