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

Support required nodes #174

Closed
kyrofa opened this Issue Feb 4, 2019 · 10 comments

Comments

Projects
None yet
5 participants
@kyrofa
Copy link
Contributor

kyrofa commented Feb 4, 2019

Feature request

Feature description

It's fairly common to want to shut down a launched system if a specific node (or set of nodes) exits. In ROS1 this uses the required attribute in the node tag. In ROS2 launch, expressing this desire requires a bit more boilerplate:

# ...
def generate_launch_description():
    talker = launch_ros.actions.Node(
        package="talker_cpp",
        node_executable="talker",
        node_name="talker",
        output="screen")

    listener = launch_ros.actions.Node(
        package="listener_py",
        node_executable="listener",
        node_name="listener",
        output="screen")

    return launch.LaunchDescription([
            talker,
            listener,

            # Boilerplate
            launch.actions.RegisterEventHandler(
                event_handler=launch.event_handlers.OnProcessExit(
                    target_action=listener,
                    on_exit=[
                        launch.actions.EmitEvent(event=launch.events.Shutdown()),
                    ],
                )
            )
    ])
# ...

Would be great to simplify this with some syntactic sugar.

Implementation considerations

I propose simply adding a required kwarg to launch.actions.ExecuteProcess which, if True, emits a Shutdown event upon ProcessExited to bring the launched system down. This turns the above into this:

# ...
def generate_launch_description():
    talker = launch_ros.actions.Node(
        package="talker_cpp",
        node_executable="talker",
        node_name="talker",
        output="screen")

    listener = launch_ros.actions.Node(
        package="listener_py",
        node_executable="listener",
        node_name="listener",
        output="screen",
        required=True)

    return launch.LaunchDescription([
            talker,
            listener
    ])
# ...

kyrofa added a commit to kyrofa/launch that referenced this issue Feb 4, 2019

Support required nodes
Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
This use-case is pretty common; add some syntactic sugar to make it a
bit easier and reduce boilerplate.

Fix ros2#174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa added a commit to kyrofa/launch that referenced this issue Feb 4, 2019

Support required nodes
Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
This use-case is pretty common; add some syntactic sugar to make it a
bit easier and reduce boilerplate.

Resolve ros2#174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

@tfoote tfoote added the in review label Feb 4, 2019

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Feb 5, 2019

One concern with the approach is the interaction with required and respawn, what if I put true for both?

This question leads me to think there may need to be one option, like an "exit handler" (a concept which was in the legacy ROS 2 launch design that didn't make it into this code yet) and only one can be in use at a time. Then there can be some default ones like Required and Respawn, which can have settings (like maybe respawn only works if the return code is in a certain set of appropriate return codes, etc.).

@kyrofa

This comment has been minimized.

Copy link
Contributor Author

kyrofa commented Feb 5, 2019

@wjwwood interesting question. How was that handled in ROS1? Was it well-received, or would you like to improve upon it?

What if respawn worked more like systemd's Restart=on-failure, where the node is only restarted if it actually exited non-zero? In that sense, both required and respawn would have meaning, even together, and it seems intuitive:

  • required: If the node exits for any reason, the launched system is brought down.
  • respawn: If the node exits non-zero, it is restarted.
  • required and respawn: If the node exits non-zero, it is restarted (without touching the rest of the launched system). If the node exits successfully, the launched system is brought down.
@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Feb 5, 2019

How was that handled in ROS1?

I'm not sure what ROS 1 does, my guess is that required overrides respawn, but I don't know. ROS 1 does, however, have respawn_delay, see: https://wiki.ros.org/roslaunch/XML/node#Attributes

Was it well-received, or would you like to improve upon it?

I think it was reasonably well received, but I saw a lot of people writing shell scripts to get the more sophisticated behaviors, and I think taking that responsibility into launch is reasonable, because it lends itself to introspection whereas scripts wrapping the nodes are opaque to tools that look at launch files.

Also, required and respawn are the only "dynamic" or "reactive" features in roslaunch from ROS 1, and the event system in launch for ROS 2 was intended to expand the horizons of that, so I guess I would like to improve upon it.

What if respawn worked more like systemd's Restart=on-failure, where the node is only restarted if it actually exited non-zero? In that sense, both required and respawn would have meaning, even together, and it seems intuitive...

I think the restart on failure combined with required makes sense, but I think they need to be aware of one another e.g. required shouldn't shutdown unless respawn declines to restart the process, which may be complicated if respawn wants to delay before restarting. That reaffirms my feelings toward a single handler, with a few different classes like Required, Respawn, and RequiredWithRespawn, that way you can have clear interactions with some behaviors, without allowing unknown interactions between new kinds of exit handlers.

But I'm not 100% convinced that's the right way to do it, I'm interested in what others think about it.

@kyrofa

This comment has been minimized.

Copy link
Contributor Author

kyrofa commented Feb 5, 2019

I think the restart on failure combined with required makes sense, but I think they need to be aware of one another e.g. required shouldn't shutdown unless respawn declines to restart the process, which may be complicated if respawn wants to delay before restarting. That reaffirms my feelings toward a single handler, with a few different classes like Required, Respawn, and RequiredWithRespawn, that way you can have clear interactions with some behaviors, without allowing unknown interactions between new kinds of exit handlers.

You're right, respawn_delay does complicates things. Not a ton though, at least not at first blush. Here's some pseudo code, is this too simple?

emit ProcessStarted
returncode = run process
emit ProcessExited

if respawn and returncode != 0:
    if respawn_delay:
        sleep respawn_delay
    re-execute
elif required:
    emit Shutdown

I don't disagree that the handler idea is powerful, however, I'm concerned that it may be over-engineered. The reason I logged this issue in the first place is because it took some boilerplate to do what I felt was a common thing. In my humble opinion, common things should be easy to do. My interpretation of what you're saying sounds like more boilerplate to do common things.

However, let me try to understand what you're saying a bit better. Would you add an on_exit parameter to ExecuteProcess? And create a set of launch.EventHandlers like:

  • Required: Simply emits Shutdown when triggered
  • Respawn: Restart process upon exit (exit codes and delay would be configurable)
  • RequiredWithRespawn: Combination of both: if process is not restarted, emit Shutdown

Hmm.... maybe they would actually be actions instead of event handlers. Still getting familiar with the code.

Would that turn the code into this?

# ...
def generate_launch_description():
    talker = launch_ros.actions.Node(
        package="talker_cpp",
        node_executable="talker",
        node_name="talker",
        output="screen")

    listener = launch_ros.actions.Node(
        package="listener_py",
        node_executable="listener",
        node_name="listener",
        output="screen",
        on_exit=launch.event_handlers.Required)

    return launch.LaunchDescription([
            talker,
            listener
    ])
# ...

That feels quite a bit less intuitive to me, is that the direction you were thinking?

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Feb 5, 2019

That feels quite a bit less intuitive to me, is that the direction you were thinking?

That was what I was thinking. I don't personally find it less intuitive than required:bool or respawn:bool, but I'm often not the best person ask about such things.

If you compare to systemd, they also use a "choice" rather than a set of boolean flags to express what they want.

Perhaps a better example, I find this:

# ...
def generate_launch_description():
    talker = launch_ros.actions.Node(
        package="talker_cpp",
        node_executable="talker",
        node_name="talker",
        output="screen")

    listener = launch_ros.actions.Node(
        package="listener_py",
        node_executable="listener",
        node_name="listener",
        output="screen",
        on_exit=launch.event_handlers.Respawn(delay=5))

    return launch.LaunchDescription([
            talker,
            listener
    ])
# ...

to be better than:

# ...
def generate_launch_description():
    talker = launch_ros.actions.Node(
        package="talker_cpp",
        node_executable="talker",
        node_name="talker",
        output="screen")

    listener = launch_ros.actions.Node(
        package="listener_py",
        node_executable="listener",
        node_name="listener",
        output="screen",
        respawn=True,
        respawn_delay=5)

# or what about
#     respawn=False,
#     respawn_delay=5)
# or just
#     respawn_delay=5)

    return launch.LaunchDescription([
            talker,
            listener
    ])
# ...
@kyrofa

This comment has been minimized.

Copy link
Contributor Author

kyrofa commented Feb 5, 2019

Fair enough. I don't completely agree, but I trust your vision! Let me take a crack at it. I'll start with the "required" functionality and split the respawn bits into another PR once the "required" bits are in, should be easier to write and review that way.

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Feb 5, 2019

Hmm.... maybe they would actually be actions instead of event handlers. Still getting familiar with the code.

They could be actions, of the form:

my_program = ExecuteProcess(...)
ld.add_action(my_program)
ld.add_action(Required(target_action=my_program))

But I actually prefer them as a new concept specific to launch.actions.ExecuteProcess and its derived classes, which is passed as the exit_handler or on_exit.

Fair enough. I don't completely agree, but I trust your vision! Let me take a crack at it. I'll start with the "required" functionality and split the respawn bits into another PR once the "required" bits are in, should be easier to write and review that way.

Well, I stand by my opinion on it, but I don't mean to be a design dictator, and so I'm open to continuing to discuss it.

I appreciate your willingness to iterate on it!

@jacobperron

This comment has been minimized.

Copy link
Member

jacobperron commented Feb 6, 2019

+1 for the event handler solution.
What about adding syntax sugar for common event handlers to the XML front-end (#163)? I imagine the XML front-end will be more common for creating launch files (as it is in ROS 1) and so it's probably worth having short-hands for required=true and respawn=5, for example.

@kyrofa

This comment has been minimized.

Copy link
Contributor Author

kyrofa commented Feb 6, 2019

You're right @jacobperron, I agree. Once the XML is in place I suspect that will be what most folks use.

@cottsay cottsay added in progress and removed in review labels Feb 7, 2019

kyrofa added a commit to kyrofa/launch that referenced this issue Feb 7, 2019

Support required nodes
Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
Instead of requiring this boilerplate for a common use-case, add an
`on_exit` parameter to `ExecuteProcess` that can take actions, and add a
new action called `Shutdown` that immediately causes the launched system
to shut down. This allows for the concept of a required node to be
expressed simply with:

    launch_ros.actions.Node(
        # ...
        on_exit=Shutdown()
    )

Resolve ros2#174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa added a commit to kyrofa/launch that referenced this issue Feb 7, 2019

Support required nodes
Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
Instead of requiring this boilerplate for a common use-case, add an
`on_exit` parameter to `ExecuteProcess` that can take actions, and add a
new action called `Shutdown` that immediately causes the launched system
to shut down. This allows for the concept of a required node to be
expressed simply with:

    launch_ros.actions.Node(
        # ...
        on_exit=Shutdown()
    )

Resolve ros2#174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa added a commit to kyrofa/launch that referenced this issue Feb 7, 2019

Support required nodes
Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
Instead of requiring this boilerplate for a common use-case, add an
`on_exit` parameter to `ExecuteProcess` that can take actions, and add a
new action called `Shutdown` that immediately causes the launched system
to shut down. This allows for the concept of a required node to be
expressed simply with:

    launch_ros.actions.Node(
        # ...
        on_exit=Shutdown()
    )

Resolve ros2#174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa added a commit to kyrofa/launch that referenced this issue Feb 7, 2019

Support required nodes
Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
Instead of requiring this boilerplate for a common use-case, add an
`on_exit` parameter to `ExecuteProcess` that can take actions, and add a
new action called `Shutdown` that immediately causes the launched system
to shut down. This allows for the concept of a required node to be
expressed simply with:

    launch_ros.actions.Node(
        # ...
        on_exit=Shutdown()
    )

Resolve ros2#174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@kyrofa

This comment has been minimized.

Copy link
Contributor Author

kyrofa commented Feb 7, 2019

Alright, the new approach is up in #179, thoughts welcome.

@tfoote tfoote added in review and removed in progress labels Feb 7, 2019

@wjwwood wjwwood closed this in #179 Feb 8, 2019

wjwwood added a commit that referenced this issue Feb 8, 2019

Support required nodes (#179)
* Support required nodes

Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
Instead of requiring this boilerplate for a common use-case, add an
`on_exit` parameter to `ExecuteProcess` that can take actions, and add a
new action called `Shutdown` that immediately causes the launched system
to shut down. This allows for the concept of a required node to be
expressed simply with:

    launch_ros.actions.Node(
        # ...
        on_exit=Shutdown()
    )

Resolve #174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

* Inherit from EmitEvent

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

* Properly align type

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

* Expose reason

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

@wjwwood wjwwood removed the in review label Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.