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


Lee Skillen <lskillen@...>
 

- 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

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