Skip to content
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

Jump to bottom

Fix executor to avoid random exceptions when shutting down #1212

Merged
merged 2 commits into from
Feb 3, 2021
Merged

Fix executor to avoid random exceptions when shutting down #1212

merged 2 commits into from
Feb 3, 2021

Conversation

ivanpauno
Copy link
Collaborator

This PR together with ros2/rclcpp#1505 seems to fix test_node failures.

Fixes #1169.
Replaces #1197.

@ivanpauno
Copy link
Collaborator Author

As a reference of the issue of using rclcpp::shutdown, see ros2/rclcpp#1139.

@ivanpauno
Copy link
Collaborator Author

@jacobperron can you review this?

jacobperron
jacobperron reviewed Jan 14, 2021
View reviewed changes
Copy link
Collaborator

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I haven't tried it out yet, but makes sense to me.

gazebo_ros/src/executor.cpp Show resolved Hide resolved
gazebo_ros/src/executor.cpp Outdated Show resolved Hide resolved
@ivanpauno ivanpauno mentioned this pull request Jan 14, 2021
Closed
@ivanpauno
Add link to related issue in rclcpp
018c260 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
jacobperron
jacobperron approved these changes Jan 20, 2021
View reviewed changes
Copy link
Collaborator

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, seems like the best we can do until some refactoring happens to the rclcpp executor.

@jacobperron
Copy link
Collaborator

@osrf-jenkins run tests please

@ivanpauno
Copy link
Collaborator Author

@jacobperron feel free to merge when this is ready, as I don't have write access here.

@jacobperron
Copy link
Collaborator

@osrf-jenkins run tests please

@jacobperron jacobperron merged commit 328243f into ros-simulation:ros2 Feb 3, 2021
This was referenced Feb 3, 2021
Closed
Closed
jacobperron pushed a commit that referenced this pull request Feb 3, 2021
@ivanpauno @jacobperron
Fix executor to avoid random exceptions when shutting down (#1212)
35172df 
* Fix executor to avoid random exceptions when shutting down

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Add link to related issue in rclcpp

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
This was referenced Feb 3, 2021
Merged
Closed
@scpeters
Copy link
Member

scpeters commented Feb 10, 2021 •
edited
Loading

the devel job on build.ros2.org has been having lots of test failures since this was merged:

any thoughts?

jacobperron
jacobperron reviewed Feb 10, 2021
View reviewed changes
gazebo_ros/src/executor.cpp
@@ -41,7 +48,9 @@ void Executor::run()

void Executor::shutdown()
{
rclcpp::shutdown();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect the issue is that we're no longer properly shutting down the context (only the executor itself). I can take a closer look.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Reverting this PR didn't seem to have any positive effect: #1227

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverting this PR didn't seem to have any positive effect: #1227

I see the revert was finally closed, is the problem fixed?
I can take another look to this if not

Copy link
Member

Choose a reason for hiding this comment

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

it was fixed by #1228

jacobperron added a commit that referenced this pull request Feb 10, 2021
@jacobperron
Revert "Fix executor to avoid random exceptions when shutting down (#…
a35a836 
…1212)"

This reverts commit 328243f.
@jacobperron jacobperron mentioned this pull request Feb 10, 2021
Closed
jacobperron pushed a commit that referenced this pull request Mar 11, 2021
@ivanpauno @jacobperron
Fix executor to avoid random exceptions when shutting down (#1212)
64f3ff1 
* Fix executor to avoid random exceptions when shutting down

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

* Add link to related issue in rclcpp

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters left review comments

@jacobperron jacobperron jacobperron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivanpauno @jacobperron @scpeters