-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
depthai::initialize() not thread or exception safe; recommend using singleton #276
Comments
Good catch @diablodale - this should've been a mutex + bool instead to block other callers in the mean time. And if you'd like, please open a PR for it, either approach seems fine. (otherwise will address by the end of the week) |
I have read the meyer single approach (these are based on c++11 "magic" statics) is the best choice. The C++ runtime and compiler manage the "magic" locks internally allowing for widespread optimization since it is a native language feature. When the same features of a meyer singleton are replicated with manual C++ code by the dev, there are less opportunities for these optimizations and most crucially more opportunity for dev mistakes/poor design. This has been my learning for years. Here is one article which is on topic https://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton showing the Meyer Singleton is almost 3 magnitudes faster than a mutex+lock_guard |
I'll push a PR. I applied my fix and it passed all 60/61 of the depthai-core test suite. The one test fails due to my OAK-D not having RGBD cali data in EEPROM.
|
(Merge defaulted to squash - seems like that's why it didn't close automatically. Closing) |
depthai::initialize()
does not handle simultaneous threads attempting toinitialize()
with blocking, and does not handle scenarios of exceptions. Easy to fix (see below)Setup
main
Repro
By code review.
depthai-core/src/utility/Initialization.cpp
Lines 48 to 55 in 8ab5bf9
Imagine multiple thread running on a multicore CPU which can concurrently run at the same time.
Scenario 1 - thread unsafe
The existing code does not distinguish between start/completed initialization. It also does not block other threads while initialization is in progress.
Scenario 2 - exceptions
The existing code does not distinguish between start/completed initialization. If something throws during initialization, then all threads are locked out and can not try to initialize again.
Expected
Thread safe, exception safe.
Threads are blocked until initialization successfully completes.
Fix
This is easily fixed with a singleton approach. Use a normal static const bool in the function and put all the code in a lambda.
This guarantees threads will block while another thread is attempting to initialize. It allows any thread to attempt init again. And it handles exceptions because c++ guarantees that if an exception occurs in such a local static var initialization, then that init doesn't occur and therefore another thread can attempt again.
For example pseudocode...
The text was updated successfully, but these errors were encountered: