Skip to content

Commit

Permalink
XML library initialisation is not thread safe (#272)
Browse files Browse the repository at this point in the history 
Note that calling XMLPlatformUtils::Initialize is currently not thread safe

Co-authored-by: Nigel Stewart <nigel.stewart@emesent.io>
  • Loading branch information
@nigels-com
nigels-com and Nigel Stewart authored Dec 7, 2023
1 parent 7cbf07d commit 4fba305
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/E57XmlParser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,8 @@ void E57XmlParser::init()
// Initialize the XML4C2 system
try
{
// NOTE: This is not thread safe for multiple simulaneous E57 readers.
// Ideally we'd do this once, not once per reader
XMLPlatformUtils::Initialize();
}
catch ( const XMLException &ex )
Expand Down

4 comments on commit 4fba305

@PerspectivesLab
Copy link

Choose a reason for hiding this comment

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

Hi @nigels-com
could the function : void E57XmlParser::init() be static ?

so we could

E57XmlParser::init();

multithreaded reading access functions here in loop

E57XmlParser::destroy()

@nigels-com
Copy link
Contributor Author

@nigels-com nigels-com commented on 4fba305 Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

One option here is C++11 call_once wrapped around the XMLPlatformUtils::Initialize call.
But that doesn't take care of cleaning up with XMLPlatformUtils::Terminate when all the readers are done.

Perhaps a static C++11 mutex protected reference count?
Initialize happens (while locked) for ++count == 1 and Terminate (while locked) when --count == 0?

@PerspectivesLab
Copy link

Choose a reason for hiding this comment

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

i guess option one,why not let the user manually handle this ? actually from xerces FAQ:

The application also needs to guarantee that the XMLPlatformUtils::Initialize() and XMLPlatformUtils::Terminate() methods are called from the same thread (usually the initial thread executing main()) or proper synchronization is performed by the application if multiple threads call XMLPlatformUtils::Initialize() and XMLPlatformUtils::Terminate() concurrently.

//main thread
XMLPlatformUtils::Initialize(); // here, is mean a static function that wraps this, so the user defines himself when to instantiate or release

// make multi-threaded work
#pragma omp parallel for
for (int i = 0; i < mFiles.size(); i++) {
  auto nthreads = omp_get_num_threads(); 
  auto id = omp_get_thread_num();
  e57::ImageFile imf(mFiles[i], "r", e57::CHECKSUM_POLICY_ALL); // Create a ReaderImpl, readers etc
  // work
  imf.close();
}

// all threads returned, cleanup
XMLPlatformUtils::Terminate(); // user is done, call this wrapped in a static function

@nigels-com
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main downside is that it's a compatibility-breaking change.

Please sign in to comment.