Skip to content

Commit

Permalink
Added tracking and cleanup for completed SCPWorker threads
Browse files Browse the repository at this point in the history
  • Loading branch information
Jim Davila committed Jun 28, 2018
1 parent 2d481c1 commit 7e00151
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 3 deletions.
4 changes: 4 additions & 0 deletions dcmnet/include/dcmtk/dcmnet/scppool.h
Expand Up @@ -232,6 +232,8 @@ class DCMTK_DCMNET_EXPORT DcmBaseSCPPool
OFList<DcmBaseSCPWorker*> m_workersBusy;
/// List of all workers being idle, i.e.\ not running a connection
OFList<DcmBaseSCPWorker*> m_workersIdle;
/// List of all completed threads
OFList<DcmBaseSCPWorker*> m_workersCompleted;

/// SCP configuration to be used by pool and all workers
DcmSCPConfig m_cfg;
Expand All @@ -251,6 +253,8 @@ class DCMTK_DCMNET_EXPORT DcmBaseSCPPool

/// Current run mode of pool
runmode m_runMode;

void cleanupCompletedThreads();
};

/** Implementation of DICOM SCP server pool. The pool waits for incoming
Expand Down
36 changes: 33 additions & 3 deletions dcmnet/libsrc/scppool.cc
Expand Up @@ -35,6 +35,7 @@ DcmBaseSCPPool::DcmBaseSCPPool()
: m_criticalSection(),
m_workersBusy(),
m_workersIdle(),
m_workersCompleted(),
m_cfg(),
m_maxWorkers(5),
m_runMode( LISTEN )
Expand Down Expand Up @@ -67,6 +68,9 @@ OFCondition DcmBaseSCPPool::listen()
/* As long as all is fine (or we have been to busy handling last connection request) keep listening */
while ( m_runMode == LISTEN && ( cond.good() || (cond == NET_EC_SCPBusy) ) )
{
// clean up any completed threads
cleanupCompletedThreads();

// Reset status
cond = EC_Normal;
// Every incoming connection is handled in a new association object
Expand Down Expand Up @@ -126,19 +130,46 @@ OFCondition DcmBaseSCPPool::listen()
{
m_criticalSection.unlock();
(*it)->join();
delete *it;
m_criticalSection.lock();
}

m_workersBusy.clear();

m_criticalSection.unlock();

cleanupCompletedThreads();

/* In the end, clean up the rest of the memory and drop network */
ASC_dropNetwork(&network);

return EC_Normal;
}

void DcmBaseSCPPool::cleanupCompletedThreads()
{

m_criticalSection.lock();
// iterate over worker threads in the list, join their threads and delete them.
for
(
OFListIterator( DcmBaseSCPPool::DcmBaseSCPWorker* ) it = m_workersCompleted.begin();
it != m_workersCompleted.end();
++it
)
{
m_criticalSection.unlock();
(*it)->join();

delete *it;

m_criticalSection.lock();
}

m_workersCompleted.clear();

m_criticalSection.unlock();
}

void DcmBaseSCPPool::stopAfterCurrentAssociations()
{
m_criticalSection.lock();
Expand Down Expand Up @@ -279,8 +310,7 @@ void DcmBaseSCPPool::notifyThreadExit(DcmBaseSCPPool::DcmBaseSCPWorker* thread,
{
DCMNET_DEBUG("DcmBaseSCPPool: Worker thread #" << thread->threadID() << " exited with error: " << result.text());
m_workersBusy.remove(thread);
delete thread;
thread = NULL;
m_workersCompleted.push_back(thread);
}
m_criticalSection.unlock();
}
Expand Down

3 comments on commit 7e00151

@zhanghu52030
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you fix the memory leak with your solution? in my side, memory leak is still existed with your fix.
almost like your solution, i also add a list to collect all the finished threads, join and delete them in listen().
did i miss something?

@pauldotknopf
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... It is working for us.

shrugs

The key thing is the join() method.

@zhanghu52030
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... It is working for us.

shrugs

The key thing is the join() method.

join them in cleanup function. right?
by the way, what do you use scp pool for? only for echo?

Please sign in to comment.