Skip to content

BUG: Improve thead safety of ObjectFactoryBase::Initialize#4226

Closed
blowekamp wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
blowekamp:fix_object_factory_unitialized
Closed

BUG: Improve thead safety of ObjectFactoryBase::Initialize#4226
blowekamp wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
blowekamp:fix_object_factory_unitialized

Conversation

@blowekamp
Copy link
Copy Markdown
Member

Block concurent threads until initializaiton is completed.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

Block concurent threads until initializaiton is completed.
@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:Core Issues affecting the Core module labels Sep 21, 2023
Comment on lines +241 to +245
if (!m_PimplGlobals->m_Initialized)
{
ObjectFactoryBase::InitializeFactoryList();
ObjectFactoryBase::RegisterInternal();
// only one thread initializing at a time, wait until initialization is completed.
std::lock_guard lockGuard(m_PimplGlobals->m_Lock);
if (!m_PimplGlobals->m_Initialized)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like you're doing double-checked locking, which is considered an anti-pattern nowadays! If one thread sets m_PimplGlobals->m_Initialized = true while at the same time, another thread tries to evaluate m_PimplGlobals->m_Initialized outside the locked region, we have undefined behavior.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What is the updated pattern? To ensure it's only initialized once and block other threads until it it initialed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

std::call_once?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My bigger concern now is if using std::mutex during static initialization is OK aka ( defined behavior ).

evaluate m_PimplGlobals->m_Initialized outside the locked region, we have undefined behavior.

Not sure how undefined the undefined read behavior would be, maybe just not 100% synced between threads. It's will either be true or false, either will be OK.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

std::call_once?

Thanks, @dzenanz. I just gave it a try, using std::call_once. Please check! (Still draft!)

std::lock_guard lockGuard(m_PimplGlobals->m_Lock);
if (!m_PimplGlobals->m_Initialized)
{
ObjectFactoryBase::InitializeFactoryList();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please consider removing the ObjectFactoryBase::InitializeFactoryList() call, as in:

@N-Dekker
Copy link
Copy Markdown
Contributor

Would it be possible to move this code (calling ObjectFactoryBase::RegisterInternal() and ObjectFactoryBase::LoadDynamicFactories()) to the default-constructor of PimplGlobals?

ObjectFactoryBase::RegisterInternal();
#if defined(ITK_DYNAMIC_LOADING) && !defined(ITK_WRAPPING)
ObjectFactoryBase::LoadDynamicFactories();
#endif

@blowekamp
Copy link
Copy Markdown
Member Author

4227 looks like a better approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants