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.

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

LoadComposableNodes action hangs #169

Closed
jacobperron opened this issue Aug 11, 2020 · 0 comments · Fixed by ros2/launch#449
Closed

LoadComposableNodes action hangs #169

jacobperron opened this issue Aug 11, 2020 · 0 comments · Fixed by ros2/launch#449
Assignees
Labels
bug Something isn't working

Comments

@jacobperron
Copy link
Member

Bug report

Required Info:

  • Operating System:
    • Ubuntu 20.04
  • Installation type:
    • from source
  • Version or commit hash:

Steps to reproduce issue

  1. Start a component container
ros2 run rclcpp_components component_container
  1. Launch the following description:
import launch

from launch_ros.actions import LoadComposableNodes
from launch_ros.descriptions import ComposableNode


def generate_launch_description():
    return launch.LaunchDescription([
        LoadComposableNodes(
            target_container='/ComponentManager', # /mock_component_container',
            composable_node_descriptions=[
                ComposableNode(
                    package='demo_nodes_cpp',
                    plugin='demo_nodes_cpp::Talker',
                )
            ]
        )
    ])

Expected behavior

Node is loaded into the container and the launch process exits.

Alternatively, if the launch process doesn't exit, sending a SIGINT or SIGQUIT should cause the launch process to exit.

Actual behavior

The node is successfully loaded, but the launch process hangs. Event SIGINT and SIGQUIT have no affect.

@jacobperron jacobperron added the bug Something isn't working label Aug 11, 2020
@jacobperron jacobperron self-assigned this Aug 11, 2020
jacobperron added a commit to ros2/launch that referenced this issue Aug 11, 2020
Fixes ros2/launch_ros#169

Otherwise, it's possible to run into a race condition between checking for 'idle' and processing events from the queue.

I.e. it's possible that an event is completed between executing the following two lines:

https://github.com/ros2/launch/blob/657745c315ba6a61984b66f168f9c34b3a7e2108/launch/launch/launch_service.py#L158

https://github.com/ros2/launch/blob/657745c315ba6a61984b66f168f9c34b3a7e2108/launch/launch/launch_service.py#L272

To fix the race, this changes makes all accesses to the event queue thread safe and introduces a helper function
get_next_event() that first checks if the queue is empty before potentially blocking forever with asynchio.Queue.get().

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/launch that referenced this issue Aug 12, 2020
…ame time

Fixes ros2/launch_ros#169

Otherwise, it's possible to get into a hung state where we wait for an event,
even though there are no more events. This is because the check for an "idle"
state evaluates to "True" as we wait for some futures to complete.

By waiting for futures and events concurrently, we can avoid this problem.
Further, we don't have to wait for an event if there's nothing in the queue.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/launch that referenced this issue Aug 13, 2020
…events (#449)

Fixes ros2/launch_ros#169

Otherwise, it's possible to get into a hung state where we wait for an event,
even though there are no more events. This is because the check for an "idle"
state evaluates to "True" as we wait for some futures to complete.

By waiting for futures and events concurrently, we can avoid this problem.
Further, we don't have to wait for an event if there's nothing in the queue.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Wait until one event is processed or nothing done (timeout)

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix bugs

Always create a task to wait on and cancel it if there is a timeout.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Avoid canceling an event mid-processing

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* minor refactor

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Guard against leaving a task pending

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Further refactoring

We don't need to have an inner loop or timeout when waiting on futures.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Only wait on futures if there are no events in the queue

This prevents spurious wake-ups when we're waiting for an event to finish processing.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/launch that referenced this issue Aug 20, 2020
…events (#449)

Fixes ros2/launch_ros#169

Otherwise, it's possible to get into a hung state where we wait for an event,
even though there are no more events. This is because the check for an "idle"
state evaluates to "True" as we wait for some futures to complete.

By waiting for futures and events concurrently, we can avoid this problem.
Further, we don't have to wait for an event if there's nothing in the queue.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Wait until one event is processed or nothing done (timeout)

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix bugs

Always create a task to wait on and cancel it if there is a timeout.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Avoid canceling an event mid-processing

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* minor refactor

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Guard against leaving a task pending

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Further refactoring

We don't need to have an inner loop or timeout when waiting on futures.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Only wait on futures if there are no events in the queue

This prevents spurious wake-ups when we're waiting for an event to finish processing.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/launch that referenced this issue Aug 27, 2020
…events (#449) (#455)

Fixes ros2/launch_ros#169

Otherwise, it's possible to get into a hung state where we wait for an event,
even though there are no more events. This is because the check for an "idle"
state evaluates to "True" as we wait for some futures to complete.

By waiting for futures and events concurrently, we can avoid this problem.
Further, we don't have to wait for an event if there's nothing in the queue.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Wait until one event is processed or nothing done (timeout)

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix bugs

Always create a task to wait on and cancel it if there is a timeout.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Avoid canceling an event mid-processing

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* minor refactor

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Guard against leaving a task pending

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Further refactoring

We don't need to have an inner loop or timeout when waiting on futures.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Only wait on futures if there are no events in the queue

This prevents spurious wake-ups when we're waiting for an event to finish processing.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant