Memory leak with DcmSCPPool<>

[theonlylawislove]

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?

[theonlylawislove]

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();

[theonlylawislove]

My team has implemented the fix.

https://github.com/MedXChange/dcmtk/com ... 6b52bfa2d3

Can we get this upstream?

[Michael Onken]

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

[Michael Onken]

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:

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

This leads to the situation where there are 0 worker threads when entering DcmBaseSCPPool::listen() and maximally m_maxWorkers during its runtime (or more exactly the method's while() loop). Once a worker is created it stays alive and is *not* joined/ended when the request it handled is done. Instead, they are being re-used. Thus, the number of threads created overall is equal to the number of simultanous association requests that occured during listen(), but maximally m_maxWorkers.

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