[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.

Signed-off-by: Lee Skillen <lskillen@...>
---
common/c_cpp/src/c/timers.c | 16 ++++++++++++++++
common/c_cpp/src/gunittest/c/timertest.cpp | 24 ++++++++++++++++++++++++
2 files changed, 40 insertions(+)

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..03e2a11 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,6 +144,26 @@ TEST_F (CommonTimerTestC, createDestroyTimer)

}

+/* 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.


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


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 


Damian Maguire <DMaguire@...>
 

Hey Lee,

I've added this patch to our bugzilla for tracking, and have put a couple
of comments on it for your feedback. If you don't mind setting yourself up
with an account, we can continue tracking the change there.

Ticket Number: http://bugs.openmama.org/show_bug.cgi?id=66


Cheers,

D




On 3/3/14 3:06 PM, "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
________________________________________________________

This message may contain confidential information and is intended for specific recipients unless explicitly noted otherwise. If you have reason to believe you are not an intended recipient of this message, please delete it and notify the sender. This message may not represent the opinion of IntercontinentalExchange Group, Inc. (ICE), NYSE Euronext or any of their subsidiaries or affiliates, and does not constitute a contract or guarantee. Unencrypted electronic mail is not secure and the recipient of this message is expected to provide safeguards from viruses and pursue alternate means of communication where privacy or a binding message is desired.
________________________________________________________