Release ownership of entities after spinning cancelled #2556

Barry-Xu-2018 · 2024-06-10T01:30:15Z

This issue was found in ros2/rmw_fastrtps#761.
In code of MoveIt, it used static executor. When the static executor is destructed (The executor is in a cancel state.), and it's time to free up the ownership of entity resources, Fast DDS resources have already been released.

After spinning is cancelled, the Executor should release ownership of entities at once.

rclcpp/src/rclcpp/executor.cpp

alsora · 2024-06-12T17:42:14Z

can we deprecate / remove the static executor rather than keep trying to fix it?
i don't see any reason why users should keep using it

wjwwood · 2024-06-12T20:20:02Z

Might be a misunderstanding, I thought they meant the StaticSingleThreadedExecutor too, but they mean they had a static-global executor, so it was destructed after the (also static-global) domain participant factory in fastrtps. At least that's my understanding.

So this affects all of the executors that use this part of the executor base class.

wjwwood · 2024-06-12T20:20:37Z

But yes, we can deprecate it in rolling at least, though that should be a separate pr.

Barry-Xu-2018 · 2024-06-13T01:57:05Z

Might be a misunderstanding, I thought they meant the StaticSingleThreadedExecutor too, but they mean they had a static-global executor, so it was destructed after the (also static-global) domain participant factory in fastrtps. At least that's my understanding.

Your understanding are right. Sorry, my description static executor caused a misunderstanding.

Barry-Xu-2018 · 2024-06-13T03:09:41Z

@wjwwood

How about moving release action to every exit point in different spin functions ?
0b4ab7e

rclcpp/test/rclcpp/test_executor.cpp

rclcpp/src/rclcpp/executor.cpp

fujitatomoya · 2024-06-13T05:42:21Z

Might be a misunderstanding, I thought they meant the StaticSingleThreadedExecutor too, but they mean they had a static-global executor, so it was destructed after the (also static-global) domain participant factory in fastrtps. At least that's my understanding.

i thought that too until looking into the reproducible program.

So this affects all of the executors that use this part of the executor base class.

true. actually they uses SingleThreadedExecutor, https://github.com/moveit/moveit_task_constructor/blob/bb047c894cd2a395a672abdd47ec76d709d1cb1e/core/src/introspection.cpp#L160

wjwwood · 2024-06-13T16:48:57Z

@Barry-Xu-2018 that was kind of what I was thinking, but we should take care to clear it before setting the spinning to false, so that a new spin doesn't enter before it can be reset.

alsora · 2024-06-13T18:15:29Z

rclcpp/src/rclcpp/executor.cpp

@@ -275,7 +275,7 @@ Executor::spin_until_future_complete_impl(
  if (spinning.exchange(true)) {
    throw std::runtime_error("spin_until_future_complete() called while already spinning");
  }
-  RCPPUTILS_SCOPE_EXIT(this->spinning.store(false); );
+  RCPPUTILS_SCOPE_EXIT(this->spinning.store(false);wait_result_.reset(););


this order is not thread-safe.
you are first setting spinning to false and then resetting the wait_result.

If there are two threads, one trying to start spin and the other exiting spin, there can be a concurrent access to wait_result_.

thread 1 is exiting spin.

thread 1 sets spinning to false

thread 2 is entering spin

thread 2 does something on the wait result

thread 1 finishes to exit and resets the wait result

We should at least invert the order, to ensure that no other threads can enter spin until we have cleared the wait result.

Thanks. It is better to invert the order.

Besides, There will be many issues if multiple threads executes the same executor's spin concurrently.
wait_result_ was completely unprotected.

rclcpp/rclcpp/src/rclcpp/executor.cpp

Lines 724 to 751 in f4923c6

void

Executor::wait_for_work(std::chrono::nanoseconds timeout)

{

TRACETOOLS_TRACEPOINT(rclcpp_executor_wait_for_work, timeout.count());

// Clear any previous wait result

this->wait_result_.reset();

{

std::lock_guard<std::mutex> guard(mutex_);

if (this->entities_need_rebuild_.exchange(false) || current_collection_.empty()) {

this->collect_entities();

}

}

this->wait_result_.emplace(wait_set_.wait(timeout));

if (!this->wait_result_ || this->wait_result_->kind() == WaitResultKind::Empty) {

RCUTILS_LOG_WARN_NAMED(

"rclcpp",

"empty wait set received in wait(). This should never happen.");

} else {

if (this->wait_result_->kind() == WaitResultKind::Ready && current_notify_waitable_) {

auto & rcl_wait_set = this->wait_result_->get_wait_set().get_rcl_wait_set();

if (current_notify_waitable_->is_ready(rcl_wait_set)) {

current_notify_waitable_->execute(current_notify_waitable_->take_data());

}

}

}

}

At least in the case of the multithreaded executor, the wait_result is protected by a mutex in the mt executor itself (

rclcpp/rclcpp/src/rclcpp/executors/multi_threaded_executor.cpp

Line 88 in f4923c6

std::lock_guard wait_lock{wait_mutex_};

)

As for single-threaded executor, it should generally not be possible to call spin from multiple threads as there is an internal spinning atomic that should block other threads from spinning an executor that is already spinning.

@mjcarroll Thanks for your comments.
I didn't express myself clearly. I mean, For the SingleThreadedExecutor, if the user creates multiple threads to execute spin(), problems will arise.

Barry-Xu-2018 · 2024-06-18T02:06:22Z

The failure of Rpr__rclcpp__ubuntu_noble_amd64 is related to test create_two_content_filters_with_same_topic_name_and_destroy. rmw_fastrtps in CI environment doesn't include ros2/rmw_fastrtps#762.

Barry-Xu-2018 · 2024-06-18T02:09:50Z

@wjwwood @alsora I have updated code. Please review again.

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 · 2024-07-03T03:01:42Z

Rebase was done.

rclcpp/test/rclcpp/test_executor.cpp

Signed-off-by: Barry Xu <[email protected]>

rclcpp/test/rclcpp/test_executor.cpp

Barry-Xu-2018 · 2024-07-03T08:24:24Z

@alsora I updated code. Please help to review again.

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 · 2024-07-04T05:15:02Z

@alsora I have moved test code. Please review again.

fujitatomoya · 2024-07-05T02:15:10Z

CI:

Linux
Linux-aarch64
Linux-rhel
Windows

Barry-Xu-2018 · 2024-07-06T10:02:19Z

CI:

Windows

fujitatomoya

lgtm

fujitatomoya · 2024-07-10T02:06:57Z

@Mergifyio backport jazzy

mergify · 2024-07-10T02:07:12Z

backport jazzy

✅ Backports have been created

#2580 Release ownership of entities after spinning cancelled (backport #2556) has been created for branch jazzy but encountered conflicts

* Release ownership of entities after spinning cancelled Signed-off-by: Barry Xu <[email protected]> * Move release action to every exit point in different spin functions Signed-off-by: Barry Xu <[email protected]> * Move wait_result_.reset() before setting spinning to false Signed-off-by: Barry Xu <[email protected]> * Update test code Signed-off-by: Barry Xu <[email protected]> * Move test code to test_executors.cpp Signed-off-by: Barry Xu <[email protected]> --------- Signed-off-by: Barry Xu <[email protected]> (cherry picked from commit 069a001) # Conflicts: # rclcpp/test/rclcpp/executors/test_executors.cpp

… (#2580) * Release ownership of entities after spinning cancelled (#2556) * Release ownership of entities after spinning cancelled Signed-off-by: Barry Xu <[email protected]> * Move release action to every exit point in different spin functions Signed-off-by: Barry Xu <[email protected]> * Move wait_result_.reset() before setting spinning to false Signed-off-by: Barry Xu <[email protected]> * Update test code Signed-off-by: Barry Xu <[email protected]> * Move test code to test_executors.cpp Signed-off-by: Barry Xu <[email protected]> --------- Signed-off-by: Barry Xu <[email protected]> (cherry picked from commit 069a001) # Conflicts: # rclcpp/test/rclcpp/executors/test_executors.cpp * Fix backport issue (#2581) Signed-off-by: Barry Xu <[email protected]> --------- Signed-off-by: Barry Xu <[email protected]> Co-authored-by: Barry Xu <[email protected]>

Barry-Xu-2018 requested review from ivanpauno, hidmic and wjwwood as code owners June 10, 2024 01:30

Barry-Xu-2018 mentioned this pull request Jun 10, 2024

Regression in Rolling: error "cannot publish data, at ./src/rmw_publish.cpp:66" during shutdown of binary ros2/rmw_fastrtps#761

Closed

wjwwood reviewed Jun 12, 2024

View reviewed changes

rclcpp/src/rclcpp/executor.cpp Outdated Show resolved Hide resolved

fujitatomoya reviewed Jun 13, 2024

View reviewed changes

rclcpp/test/rclcpp/test_executor.cpp Outdated Show resolved Hide resolved

rclcpp/src/rclcpp/executor.cpp Outdated Show resolved Hide resolved

rclcpp/src/rclcpp/executor.cpp Outdated Show resolved Hide resolved

alsora reviewed Jun 13, 2024

View reviewed changes

wjwwood mentioned this pull request Jun 18, 2024

add test to check for execution order of entities in various executors #2536

Closed

fujitatomoya self-assigned this Jun 27, 2024

fujitatomoya requested review from wjwwood and mjcarroll June 27, 2024 23:44

Barry-Xu-2018 added 3 commits July 3, 2024 11:00

Release ownership of entities after spinning cancelled

118773c

Signed-off-by: Barry Xu <[email protected]>

Move release action to every exit point in different spin functions

429fc32

Signed-off-by: Barry Xu <[email protected]>

Move wait_result_.reset() before setting spinning to false

50bce78

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 force-pushed the review/topic-fix-cancel-spin-issue branch from b30df3a to 50bce78 Compare July 3, 2024 03:01

alsora reviewed Jul 3, 2024

View reviewed changes

rclcpp/test/rclcpp/test_executor.cpp Outdated Show resolved Hide resolved

alsora reviewed Jul 3, 2024

View reviewed changes

rclcpp/test/rclcpp/test_executor.cpp Outdated Show resolved Hide resolved

Update test code

815de21

Signed-off-by: Barry Xu <[email protected]>

Barry-Xu-2018 commented Jul 3, 2024

View reviewed changes

rclcpp/test/rclcpp/test_executor.cpp Outdated Show resolved Hide resolved

Move test code to test_executors.cpp

cc70aad

Signed-off-by: Barry Xu <[email protected]>

alsora approved these changes Jul 6, 2024

View reviewed changes

fujitatomoya approved these changes Jul 10, 2024

View reviewed changes

fujitatomoya merged commit 069a001 into ros2:rolling Jul 10, 2024
3 checks passed

mergify bot mentioned this pull request Jul 10, 2024

Release ownership of entities after spinning cancelled (backport #2556) #2580

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release ownership of entities after spinning cancelled #2556

Release ownership of entities after spinning cancelled #2556

Barry-Xu-2018 commented Jun 10, 2024

alsora commented Jun 12, 2024

wjwwood commented Jun 12, 2024

wjwwood commented Jun 12, 2024

Barry-Xu-2018 commented Jun 13, 2024

Barry-Xu-2018 commented Jun 13, 2024

fujitatomoya commented Jun 13, 2024

wjwwood commented Jun 13, 2024

alsora Jun 13, 2024 •

edited

Loading

Barry-Xu-2018 Jun 14, 2024

mjcarroll Jun 17, 2024

Barry-Xu-2018 Jun 18, 2024

Barry-Xu-2018 commented Jun 18, 2024

Barry-Xu-2018 commented Jun 18, 2024

Barry-Xu-2018 commented Jul 3, 2024

Barry-Xu-2018 commented Jul 3, 2024

Barry-Xu-2018 commented Jul 4, 2024

fujitatomoya commented Jul 5, 2024

Barry-Xu-2018 commented Jul 6, 2024

fujitatomoya left a comment

fujitatomoya commented Jul 10, 2024

mergify bot commented Jul 10, 2024 •

edited

Loading

	void
	Executor::wait_for_work(std::chrono::nanoseconds timeout)
	{
	TRACETOOLS_TRACEPOINT(rclcpp_executor_wait_for_work, timeout.count());

	// Clear any previous wait result
	this->wait_result_.reset();

	{
	std::lock_guard<std::mutex> guard(mutex_);
	if (this->entities_need_rebuild_.exchange(false) \|\| current_collection_.empty()) {
	this->collect_entities();
	}
	}
	this->wait_result_.emplace(wait_set_.wait(timeout));
	if (!this->wait_result_ \|\| this->wait_result_->kind() == WaitResultKind::Empty) {
	RCUTILS_LOG_WARN_NAMED(
	"rclcpp",
	"empty wait set received in wait(). This should never happen.");
	} else {
	if (this->wait_result_->kind() == WaitResultKind::Ready && current_notify_waitable_) {
	auto & rcl_wait_set = this->wait_result_->get_wait_set().get_rcl_wait_set();
	if (current_notify_waitable_->is_ready(rcl_wait_set)) {
	current_notify_waitable_->execute(current_notify_waitable_->take_data());
	}
	}
	}
	}

Release ownership of entities after spinning cancelled #2556

Release ownership of entities after spinning cancelled #2556

Conversation

Barry-Xu-2018 commented Jun 10, 2024

alsora commented Jun 12, 2024

wjwwood commented Jun 12, 2024

wjwwood commented Jun 12, 2024

Barry-Xu-2018 commented Jun 13, 2024

Barry-Xu-2018 commented Jun 13, 2024

fujitatomoya commented Jun 13, 2024

wjwwood commented Jun 13, 2024

alsora Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Barry-Xu-2018 Jun 14, 2024

Choose a reason for hiding this comment

mjcarroll Jun 17, 2024

Choose a reason for hiding this comment

Barry-Xu-2018 Jun 18, 2024

Choose a reason for hiding this comment

Barry-Xu-2018 commented Jun 18, 2024

Barry-Xu-2018 commented Jun 18, 2024

Barry-Xu-2018 commented Jul 3, 2024

Barry-Xu-2018 commented Jul 3, 2024

Barry-Xu-2018 commented Jul 4, 2024

fujitatomoya commented Jul 5, 2024

Barry-Xu-2018 commented Jul 6, 2024

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Jul 10, 2024

mergify bot commented Jul 10, 2024 • edited Loading

✅ Backports have been created

alsora Jun 13, 2024 •

edited

Loading

mergify bot commented Jul 10, 2024 •

edited

Loading