-
Notifications
You must be signed in to change notification settings - Fork 142
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
Ensure that shutdown event is handled correctly #154
Conversation
There is a potential race condition in bewteen when the shutdown event is emitted and the rest of the shutdown handling code. This introduces an additional await to ensure that the event is emitted before proceeding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -57,7 +57,7 @@ def _assert_type_error_creating_node(self, *, parameters=None, remappings=None): | |||
def test_launch_invalid_node(self): | |||
"""Test launching an invalid node.""" | |||
node_action = launch_ros.actions.Node( | |||
package='nonexistent_package', node_executable='node', output='screen'), | |||
package='nonexistent_package', node_executable='node', output='screen') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We think this was always causing a subtle bug (node_action
was a single item tuple rather than just a single item), but was being hidden because the tests didn't complete due to the race condition being fixed in this pull request.
There is a potential race condition in bewteen when the shutdown event
is emitted and the rest of the shutdown handling code. This introduces
an additional await to ensure that the event is emitted before
proceeding.