Skip to content

Commit

Permalink
BUG: fixed various threading errors in logging code, found by TSan
Browse files Browse the repository at this point in the history
- made some flags std::atomic so they can be safely polled/set from multiple threads
- split a method (Flush) into a public and private pair, the public method locks the mutex, the private method assumes its already held. This allows using the private method in a few places where othwise the mutex was being release/acquired back to back.
  • Loading branch information
seanm authored and dzenanz committed Sep 21, 2021
1 parent 54731ef commit 9599fc6
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 12 deletions.
3 changes: 3 additions & 0 deletions Modules/Core/Common/include/itkLoggerBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,9 @@ class ITKCommon_EXPORT LoggerBase : public Object
Flush();

protected:
virtual void
PrivateFlush();

/** Constructor */
LoggerBase();

Expand Down
6 changes: 5 additions & 1 deletion Modules/Core/Common/include/itkLoggerThreadWrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <string>
#include <queue>
#include <thread>
#include <atomic>

#include "itkObjectFactory.h"
#include <mutex>
Expand Down Expand Up @@ -134,6 +135,9 @@ class ITK_TEMPLATE_EXPORT LoggerThreadWrapper : public SimpleLoggerType
Flush() override;

protected:
void
PrivateFlush() override;

/** Constructor */
LoggerThreadWrapper();

Expand All @@ -158,7 +162,7 @@ class ITK_TEMPLATE_EXPORT LoggerThreadWrapper : public SimpleLoggerType

std::thread m_Thread;

bool m_TerminationRequested;
std::atomic<bool> m_TerminationRequested;

OperationContainerType m_OperationQ;

Expand Down
20 changes: 14 additions & 6 deletions Modules/Core/Common/include/itkLoggerThreadWrapper.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -112,18 +112,18 @@ LoggerThreadWrapper<SimpleLoggerType>::Write(PriorityLevelEnum level, std::strin
this->m_MessageQ.push(content);
this->m_LevelQ.push(level);
}
this->m_Mutex.unlock();
if (this->m_LevelForFlushing >= level)
{
this->Flush();
this->PrivateFlush();
}
this->m_Mutex.unlock();
}

template <typename SimpleLoggerType>
void
LoggerThreadWrapper<SimpleLoggerType>::Flush()
LoggerThreadWrapper<SimpleLoggerType>::PrivateFlush()
{
this->m_Mutex.lock();
// m_Mutex must already be held here!

while (!this->m_OperationQ.empty())
{
Expand All @@ -149,8 +149,16 @@ LoggerThreadWrapper<SimpleLoggerType>::Flush()
}
this->m_OperationQ.pop();
}
this->SimpleLoggerType::Flush();
this->SimpleLoggerType::PrivateFlush();
this->m_Output->Flush();
}

template <typename SimpleLoggerType>
void
LoggerThreadWrapper<SimpleLoggerType>::Flush()
{
this->m_Mutex.lock();
this->PrivateFlush();
this->m_Mutex.unlock();
}

Expand Down Expand Up @@ -209,8 +217,8 @@ LoggerThreadWrapper<SimpleLoggerType>::ThreadFunction()
}
m_OperationQ.pop();
}
SimpleLoggerType::PrivateFlush();
m_Mutex.unlock();
SimpleLoggerType::Flush();
itksys::SystemTools::Delay(this->GetDelay());
}
}
Expand Down
3 changes: 2 additions & 1 deletion Modules/Core/Common/include/itkThreadLogger.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include "itkLogger.h"
#include <mutex>
#include <atomic>

#include <string>
#include <queue>
Expand Down Expand Up @@ -135,7 +136,7 @@ class ITKCommon_EXPORT ThreadLogger : public Logger

std::thread m_Thread;

bool m_TerminationRequested;
std::atomic<bool> m_TerminationRequested;

OperationContainerType m_OperationQ;

Expand Down
6 changes: 6 additions & 0 deletions Modules/Core/Common/src/itkLoggerBase.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ LoggerBase::Write(PriorityLevelEnum level, std::string const & content)

void
LoggerBase::Flush()
{
this->PrivateFlush();
}

void
LoggerBase::PrivateFlush()
{
this->m_Output->Flush();
}
Expand Down
7 changes: 3 additions & 4 deletions Modules/Core/Common/src/itkThreadLogger.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -108,26 +108,26 @@ ThreadLogger::Write(PriorityLevelEnum level, std::string const & content)
this->m_OperationQ.push(WRITE);
this->m_MessageQ.push(content);
this->m_LevelQ.push(level);
this->m_Mutex.unlock();
if (this->m_LevelForFlushing >= level)
{
this->InternalFlush();
}
this->m_Mutex.unlock();
}

void
ThreadLogger::Flush()
{
this->m_Mutex.lock();
this->m_OperationQ.push(FLUSH);
this->m_Mutex.unlock();
this->InternalFlush();
this->m_Mutex.unlock();
}

void
ThreadLogger::InternalFlush()
{
this->m_Mutex.lock();
// m_Mutex must already be held here!

while (!this->m_OperationQ.empty())
{
Expand Down Expand Up @@ -160,7 +160,6 @@ ThreadLogger::InternalFlush()
this->m_OperationQ.pop();
}
this->m_Output->Flush();
this->m_Mutex.unlock();
}

void
Expand Down

0 comments on commit 9599fc6

Please sign in to comment.