I while back, I identified a memory leak in DcmSCPPool. Here was that thread for more information: https://forum.dcmtk.org/viewtopic.php?f=1&t=4534
For other reasons, I was forced to dig in and figure out that leak, and I have identified the cause.
The problem is that when you do pthread_create, you MUST pthread_join on it. I understand that within the thread func, we are calling pthread_exit, but someone must await that thread with pthread_join, or you will get memory leaks.
The DcmSCPPool network thread currently does a "fire and forgot" approach to spawning the threads. It creates a new thread, a sense the association on it's way. The new thread will eventually remove itself from m_workersBusy, but that isn't enough. Another thread must call pthread_join.
You cannot call pthread_join within DcmBaseSCPWorker::notifyThreadExit because that get's called on the spawned thread directly, so we'd get a deadlock.
We are currently working on a patch to solve this, but I want to go over it with you guys so that it could potentially be merged upstream.
Check this loop: https://github.com/MedXChange/dcmtk/blo ... ool.cc#L73
Instead of deleting the thread within notifyThreadExit, I will add the thread to a new OFList, m_workersDead.
Within this loop, I will check for the dead threads, and simply call "deadThread->join()" on them, and then actually delete them. It will take no time because the thread will have already completed, making pthread_join simple return immediately.
I have tested this solution and verified it fixes the memory leak.
What do you guys think?
Memory leak with DcmSCPPool<>
Moderator: Moderator Team
-
- Posts: 34
- Joined: Thu, 2014-07-17, 09:07
Re: Memory leak with DcmSCPPool<>
Actually, I found a better solution. Here is my actual problem:
https://stackoverflow.com/questions/176 ... emory-leak
You can fire-and-forgot pthreads, but you must call pthread_detatch after creating it.
I suggest adding a OFThread::detach() method that callers can use if they expect to call OFThread::start() without ever calling OFThread::join();
https://stackoverflow.com/questions/176 ... emory-leak
You can fire-and-forgot pthreads, but you must call pthread_detatch after creating it.
I suggest adding a OFThread::detach() method that callers can use if they expect to call OFThread::start() without ever calling OFThread::join();
-
- Posts: 34
- Joined: Thu, 2014-07-17, 09:07
Re: Memory leak with DcmSCPPool<>
My team has implemented the fix.
https://github.com/MedXChange/dcmtk/com ... 6b52bfa2d3
Can we get this upstream?
https://github.com/MedXChange/dcmtk/com ... 6b52bfa2d3
Can we get this upstream?
-
- DCMTK Developer
- Posts: 2051
- Joined: Fri, 2004-11-05, 13:47
- Location: Oldenburg, Germany
- Contact:
Re: Memory leak with DcmSCPPool<>
Hi,
sorry for the late esponse. We'll look into it and if there is a problem and your bug is fixing it in a "DCMTK-compatible" mail, we're taking this upstream, sure!
Thanks for the report and work so far.
Best regards,
Michael
sorry for the late esponse. We'll look into it and if there is a problem and your bug is fixing it in a "DCMTK-compatible" mail, we're taking this upstream, sure!
Thanks for the report and work so far.
Best regards,
Michael
-
- DCMTK Developer
- Posts: 2051
- Joined: Fri, 2004-11-05, 13:47
- Location: Oldenburg, Germany
- Contact:
Re: Memory leak with DcmSCPPool<>
I finally had time to look at the patch -- my impression is that it is not necessary to cleanup completed threads while in the listening loop.
The implementation currently works this way:
However, you are right, of course every thread must be joined once somewhen. This is done right after the while() loop in DcmBaseSCPPool::listen()).
Thus there should not be any unjoined thread once DcmBaseSCPPool::listen() exits.
Do I miss something?
Best regards,
Michael
The implementation currently works this way:
- Start listening (DcmBaseSCPPool::listen()) and keep listening as long as no stop is requested.
- If a request is arriving, check whether we have idle workers.
- If not, there are 3 possibilities:
- We have idle worker threads, then use one of them, otherwise
- Check whether we can create one (i.e. configured maximum number of threads is not reached) and if so create it and let it handle the request,
- And as a last resort, do nothing and keep listening, i.e. wait that a worker thread becomes idle.
However, you are right, of course every thread must be joined once somewhen. This is done right after the while() loop in DcmBaseSCPPool::listen()).
Thus there should not be any unjoined thread once DcmBaseSCPPool::listen() exits.
Do I miss something?
Best regards,
Michael
Who is online
Users browsing this forum: Bing [Bot], Google [Bot] and 1 guest