Re: [PATCH] COMMON: Fix memory leaks in common-based timers.


Lee Skillen <lskillen@...>
 

Rerolled patch removes destroyTimer() call in qpid when resetting the timer - It was called and the timer element wasn't NULLed, causing createTimer to try to free it again (with the patched code).  I'm not sure what everyone's opinion is but it seems cleaner to let the common code destroy the timer if it already exists rather than destroying it manually first (esp. because this is a common pattern with this code, although actually a common timer_reset() function should probably be implemented instead).


On 3 March 2014 15:06, Lee Skillen <lskillen@...> wrote:
- When destroying the timers heap, the dispatching thread is now joined
  and the socket pair (pipe) is closed, freeing the resources for both.

- When creating a timer, if the timer that has been passed in has been
  created before (which is usual in the onTimer/destroy/create chain of
  events), the existing timer is destroyed first to free its resources.

- Added CommonTestTimerC.createDestroySameTimer to test the above.

- QPid: Let the common code destroy the timer on resets.

Signed-off-by: Lee Skillen <lskillen@...>
---
 common/c_cpp/src/c/timers.c                | 16 ++++++++++++++++
 common/c_cpp/src/gunittest/c/timertest.cpp | 26 +++++++++++++++++++++++++-
 mama/c_cpp/src/c/bridge/qpid/timer.c       |  3 ---
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/common/c_cpp/src/c/timers.c b/common/c_cpp/src/c/timers.c
index 8cdc665..0b98984 100644
--- a/common/c_cpp/src/c/timers.c
+++ b/common/c_cpp/src/c/timers.c
@@ -242,6 +242,12 @@ writeagain:
                                &heapImpl->mEndingLock);
         }
         wthread_mutex_unlock (&heapImpl->mEndingLock);
+
+        wthread_join (heapImpl->mDispatchThread, NULL);
+
+        close (heapImpl->mSockPair[0]);
+        close (heapImpl->mSockPair[1]);
+
         free (heapImpl);
     }
     return 0;
@@ -255,6 +261,15 @@ int createTimer (timerElement* timer, timerHeap heap, timerFireCb cb, struct tim

     {
         timerHeapImpl* heapImpl = (timerHeapImpl*)heap;
+
+        if (*timer)
+        {
+            /* If the timer already exists ensure that it has been destroyed
+             * first before it is recreated again. */
+            destroyTimer (heap, *timer);
+            *timer = NULL;
+        }
+
         int kickPipe = 0;
         struct timeval now;
         timerImpl* nextTimeOut = NULL;
@@ -292,6 +307,7 @@ writeagain:
                 }
             }
         }
+
         wthread_mutex_unlock (&heapImpl->mLock);

         *timer = ele;
diff --git a/common/c_cpp/src/gunittest/c/timertest.cpp b/common/c_cpp/src/gunittest/c/timertest.cpp
index 9100895..596da9a 100644
--- a/common/c_cpp/src/gunittest/c/timertest.cpp
+++ b/common/c_cpp/src/gunittest/c/timertest.cpp
@@ -71,6 +71,10 @@ static void  onTimerFire(timerElement timer, void* closure)
     ASSERT_EQ (0, destroyTimer(closure,timer)) <<"Could not destroy timer!";
 }

+static void  onTimerFireNop(timerElement timer, void* closure)
+{
+}
+
 #if defined(__cplusplus)
 }
 #endif
@@ -140,7 +144,27 @@ TEST_F (CommonTimerTestC, createDestroyTimer)

 }

-/*  Description: Creates a timerHeap, starts dispatching  then creates
+/*  Description: Creates a timerHeap, creates more than one timer using
+ *               the same pointer to ensure the existing one is destroyed.
+ */
+TEST_F (CommonTimerTestC, createDestroySameTimer)
+{
+    struct timeval  timeout;
+    timerElement    timer   = NULL;
+    timerHeap       heap    = NULL;
+
+    timeout.tv_sec  = 0;
+    timeout.tv_usec = 500;
+
+    ASSERT_EQ (0, createTimerHeap(&heap));
+    ASSERT_EQ (0, startDispatchTimerHeap(heap));
+    ASSERT_EQ (0, createTimer(&timer, heap, onTimerFireNop, &timeout, heap));
+    ASSERT_EQ (0, createTimer(&timer, heap, onTimerFireNop, &timeout, heap));
+    ASSERT_EQ (0, destroyTimer(heap, timer));
+    ASSERT_EQ (0, destroyHeap(heap));
+}
+
+/*  Description: Creates a timerHeap, starts dispatching  then creates
  *               100 timers which destroys themselves by callback. Heap is
  *               then destroyed.
  *
diff --git a/mama/c_cpp/src/c/bridge/qpid/timer.c b/mama/c_cpp/src/c/bridge/qpid/timer.c
index d3a0f3a..f96ac02 100644
--- a/mama/c_cpp/src/c/bridge/qpid/timer.c
+++ b/mama/c_cpp/src/c/bridge/qpid/timer.c
@@ -210,9 +210,6 @@ qpidBridgeMamaTimer_reset (timerBridge timer)
         return MAMA_STATUS_NULL_ARG;
     }

-    /* Destroy the existing timer element */
-    destroyTimer (gQpidTimerHeap, impl->mTimerElement);
-
     /* Calculate next time interval */
     timeout.tv_sec  = (time_t) impl->mInterval;
     timeout.tv_usec = ((impl->mInterval- timeout.tv_sec) * 1000000.0);
--
1.9.0




--
Lee Skillen

Vulcan Financial Technologies
51 Malone Road, Belfast, BT9 6RY

Office:  +44 (0)28 95 817888
Mobile:  +44 (0)78 41 425152
Web:     www.vulcanft.com 

Join Openmama-dev@lists.openmama.org to automatically receive all group messages.