Skip to content

fix ROSspinThread shutdown data race and prune unloaded instances #361

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.

Sign up for GitHub

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

Open
ahcorde wants to merge 3 commits into
base: ahcorde/rolling/redundant_instance
Choose a base branch
from
+9 −8
Open

fix ROSspinThread shutdown data race and prune unloaded instances #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
22bfa44
fix ROSspinThread shutdown data race and prune unloaded instances
ahcorde Jun 26, 2026
cba4155
Merge remote-tracking branch 'origin/ahcorde/rolling/redundant_instan…
ahcorde Jul 10, 2026
a287b44
review feedback
ahcorde Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions rqt_gui_cpp/src/rqt_gui_cpp/nodelet_plugin_provider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ NodeletPluginProvider::NodeletPluginProvider(
NodeletPluginProvider::~NodeletPluginProvider()
{
if (ros_spin_thread_ != nullptr) {
ros_spin_thread_->abort = true;
ros_spin_thread_->abort.store(true, std::memory_order_relaxed);

@asymingt asymingt Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In the title of this PR you suggest that the addition of this line prevents a data race. However, the std::memory_order_relaxed actually adds no guardrails when it comes to compiler optimizations reordering instructions and breaking thread safety.

Why did you choose this in stead of std::memory_order_seq_cst?

@ahcorde ahcorde Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

both solution are equivalent in this case.

ros_spin_thread_->exec_.remove_node(node_);
ros_spin_thread_->wait();
ros_spin_thread_->deleteLater();
Expand All @@ -69,6 +69,7 @@ void NodeletPluginProvider::unload(void * instance)
}

qt_gui_cpp::RosPluginlibPluginProvider<rqt_gui_cpp::Plugin>::unload(instance);
instances_.remove(instance);
}

void NodeletPluginProvider::init_loader()
Expand All @@ -95,16 +96,14 @@ void NodeletPluginProvider::init_loader()

std::shared_ptr<Plugin> NodeletPluginProvider::create_plugin(
const std::string & lookup_name,
qt_gui_cpp::PluginContext * plugin_context)
qt_gui_cpp::PluginContext * /*plugin_context*/)
{
init_loader();

std::string nodelet_name = std::format("{}_{}", lookup_name, plugin_context->serialNumber());

std::shared_ptr<rqt_gui_cpp::Plugin> instance =
qt_gui_cpp::RosPluginlibPluginProvider<rqt_gui_cpp::Plugin>::create_plugin(lookup_name);
instance->passInNode(node_);
instances_[instance.get()] = nodelet_name.c_str();
instances_.insert(instance.get());

return instance;
}
Expand Down Expand Up @@ -135,7 +134,7 @@ NodeletPluginProvider::RosSpinThread::~RosSpinThread() = default;

void NodeletPluginProvider::RosSpinThread::run()
{
while (!abort) {
while (!abort.load(std::memory_order_relaxed)) {
// Spin the executor
exec_.spin_once();
}
Expand Down
6 changes: 4 additions & 2 deletions rqt_gui_cpp/src/rqt_gui_cpp/nodelet_plugin_provider.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@
#ifndef RQT_GUI_CPP__NODELET_PLUGIN_PROVIDER_HPP_
#define RQT_GUI_CPP__NODELET_PLUGIN_PROVIDER_HPP_

#include <QSet>
#include <QThread>
#include <atomic>
#include <memory>
#include <string>

Expand Down Expand Up @@ -68,7 +70,7 @@ class NodeletPluginProvider
const QString & plugin_id, qt_gui_cpp::PluginContext * plugin_context,
qt_gui_cpp::Plugin * plugin) override;

QMap<void *, QString> instances_;
QSet<void *> instances_;

bool loader_initialized_;

Expand All @@ -83,7 +85,7 @@ class NodeletPluginProvider
explicit RosSpinThread(QObject * parent = nullptr);
~RosSpinThread() override;
void run() override;
bool abort;
std::atomic<bool> abort;
// Create an executor that will be responsible for execution of callbacks for a set of nodes.
// With this version, all callbacks will be called from within this thread (the main one).
rclcpp::executors::MultiThreadedExecutor exec_;
Expand Down