search

ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
126 stars 142 forks source link

Re-spawn a process only if `returncode != 0` #514

Open lukicdarkoo opened 3 years ago

lukicdarkoo commented 3 years ago

I believe there is a missing condition returncode != 0 when respawning a process. I have a process that finishes cleanly after a short time, but the launcher keeps respawning it. With the condition (returncode != 0) it works fine.

hidmic commented 3 years ago

@lukicdarkoo have you considered:

With that in mind, I'd suggest we add support for a condition to respawn in addition to plain booleans, instead of hard-coding this check.

?

lukicdarkoo commented 3 years ago

@lukicdarkoo have you considered:

With that in mind, I'd suggest we add support for a condition to respawn in addition to plain booleans, instead of hard-coding this check.

?

Sounds very good to me, but I understood that @ivanpauno and @wjwwood have to agree or contribute to the idea?

ivanpauno commented 3 years ago

With that in mind, I'd suggest we add support for a condition to respawn in addition to plain booleans, instead of hard-coding this check. @ivanpauno @wjwwood for feedback.

Sounds fair, but how do you get the return code in the condition?

hidmic commented 3 years ago

Sounds fair, but how do you get the return code in the condition?

Hmm. Didn't put a lot of thought to it. Temporarily pushing it to context.locals is one option.

ivanpauno commented 3 years ago

Hmm. Didn't put a lot of thought to it. Temporarily pushing it to context.locals is one option.

That works, but it's really weird IMO. I would rather let the user pass a callback which takes the return code as its only argument. We could also accept a two arguments version, where the second one is the context, though I don't think that's really useful.

hidmic commented 3 years ago

I would rather let the user pass a callback which takes the return code as its only argument.

Right, but that'd make it impossible to leverage the feature from non-Python launch files.

I'm onboard with accepting callbacks for user convenience, but we have to accept a condition as well i.e. a construct that actually lives in launch conceptual domain.

ivanpauno commented 3 years ago

Okay, pushing it to context.locals sounds fair to me.

wjwwood commented 3 years ago

Okay, pushing it to context.locals sounds fair to me.

Same.

hidmic commented 3 years ago

@lukicdarkoo will you be moving forward with this?

hidmic commented 2 years ago

@lukicdarkoo friendly ping