Repository navigation
concurrent accesses well-defined - #231
Merged
Merged
Conversation
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
|
Tick the box to add this pull request to the merge queue (same as
|
Contributor
Author
|
Pulls: #231
|
asymingt
reviewed
Jul 8, 2026
There was a problem hiding this comment.
I had no idea the if with initializer existed. TIL.
thomasmoore-torc
added a commit
to thomasmoore-torc/class_loader
that referenced
this pull request
Sep 22, 2026
AbstractMetaObjectBase::addOwningClassLoader()/removeOwningClassLoader()/ isOwnedBy() (and the other accessors of associated_class_loaders_) took no lock of their own. createInstance() in class_loader_core.hpp released getPluginBaseToFactoryMapMapMutex() before reading factory->isOwnedBy(loader), so a concurrent addOwningClassLoader()/removeOwningClassLoader() on another thread (e.g. from loadLibrary()'s already-loaded branch, or from onPluginDeletion()) could race with that read - and, since it's a std::vector, with any other concurrent read/write of the same metaobject's owner list. Reported by ThreadSanitizer as a data race on std::vector<ClassLoader *>'s push_back()/erase() (via vector's internal reallocation), hit by constructing many independent ClassLoader instances for the same library concurrently - e.g. one rosbag2_cpp::Writer per thread, each internally constructing its own pluginlib::ClassLoader for the serialization format converter. Give AbstractMetaObjectBaseImpl its own recursive_mutex guarding associated_class_loaders_, so every accessor is self-contained and safe regardless of what lock (if any) the caller happens to be holding. Also extend createInstance()'s existing getPluginBaseToFactoryMapMapMutex() scope to cover the isOwnedBy() checks themselves, since between releasing that mutex and reading isOwnedBy(), the factory pointer looked up under it could in principle be invalidated by a concurrent destroyMetaObjectsForLibrary() on another thread. This is separate from ros#234 (loadLibrary()/unloadLibrary() overlap) and ros#231 (has_unmanaged_instance_been_created_): those fixed different unsynchronized globals in this same area. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: thomasmoore-torc <thomas.moore@torc.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
has_unmanaged_instance_been_created_ flag std::atomic to fix a real data race (unsynchronized read/write across per-loader mutexes) that could prematurely dlclose an in-use library.
@asymingt FYI
Claude Opus 4.7