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

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

Draft
wants to merge 4 commits into
base: rolling
Choose a base branch
from

Conversation

lukicdarkoo
Copy link

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.

Signed-off-by: Darko Lukic <lukicdarkoo@gmail.com>
Signed-off-by: Darko Lukic <lukicdarkoo@gmail.com>
Signed-off-by: Darko Lukic <lukicdarkoo@gmail.com>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

@lukicdarkoo I see you use respawn as means to retry on failure. I could argue that someone may want to use respawn to keep some process alive no matter what (current behavior in master), or to reset some process' state so long as it exits gracefully (opposite to the behavior in this patch).

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.

@@ -152,7 +152,7 @@ def generate_launch_description():
return LaunchDescription([

ExecuteProcess(
cmd=[sys.executable, '-c', "print('action')"],
cmd=[sys.executable, '-c', "print('action'); false"],
Copy link
Contributor

Choose a reason for hiding this comment

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

@lukicdarkoo hmm, false is not a valid Python literal.

Copy link
Author

Choose a reason for hiding this comment

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

Probably not the best way to make it crash :) I will change it

Signed-off-by: Darko Lukic <lukicdarkoo@gmail.com>
@hidmic
Copy link
Contributor

hidmic commented Jul 5, 2021

@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.

?

@clalancette clalancette added the more-information-needed Further information is required label Jul 22, 2021
@lukicdarkoo
Copy link
Author

@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
Copy link
Member

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
Copy link
Contributor

hidmic commented Jul 26, 2021

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
Copy link
Member

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
Copy link
Contributor

hidmic commented Jul 26, 2021

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
Copy link
Member

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

@lukicdarkoo lukicdarkoo marked this pull request as draft July 26, 2021 16:01
@wjwwood
Copy link
Member

wjwwood commented Jul 26, 2021

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

Same.

@hidmic
Copy link
Contributor

hidmic commented Oct 29, 2021

@lukicdarkoo will you be moving forward with this?

@hidmic
Copy link
Contributor

hidmic commented Mar 21, 2022

@lukicdarkoo friendly ping

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants