Return the exit code a required process through roslaunch #2082

lucasw · 2020-10-19T17:39:47Z

Return the exit code to the caller of a required process at least for a regular parent launch process, child and remote processes need looking at.

May solve #919

sloretz

May solve #919

Please add a test :)

It looks like CI is failing. Please resolve the existing test failures.

tools/roslaunch/src/roslaunch/__init__.py

lucasw · 2020-11-06T18:24:24Z

Made some cleanup and existing tests pass.

A new test that launches a node with required=true then forces it to exit non-zero, then inspects roslaunch exit code is the next step. Any pointers to an existing roslaunch test that would be a good template for this would be very welcome, otherwise I'll figure it out eventually.

I'll hold on to the remaining extra prints until I'm done with that, a few I think it makes sense to keep.

xkortex · 2021-02-01T21:24:34Z

Just tested this patch ported to melodic. Works for me!

I did notice that the roslaunch xml node has to be marked as required="true", which is logical. Just wanted to point that out, as it threw me for a loop for a second.

1612214530.892673: Code shutdown requested: 37
================================================================================REQUIRED process [host/exit_code_node-1] has died!
process has died [pid 24194, exit code 37, cmd /root/src/core/nodes/exit_code_node.py __name:=exit_code_node __log:=/root/.ros/log/906b1440-649f-11eb-8a83-78d00426c726/-exit_code_node-1.log].
log file: /root/.ros/log/906b1440-649f-11eb-8a83-78d00426c726/-exit_code_node-1*.log
Initiating shutdown!
================================================================================
Going to exit due to code 37
[host/exit_code_node-1] killing on exit
shutting down processing monitor...
... shutting down processing monitor complete
roslaunch runner done
ending server mode 37
exiting from main() 37
root@remote:~# echo $?
37

LukeAI · 2021-02-23T15:34:49Z

would love to see this in melodic. Is there something I can do to help get this merged?

… a regular parent launch process, child and remote processes need looking at. May solve ros#919

lucasw · 2021-03-21T17:19:17Z

Added a unit test, can run manually with

catkin run_tests
rostest test_roslaunch roslaunch.test

Try running lifecycle_test.launch - fails but doesn't result in a non-zero exit code (see ros/ros_comm#2082). Fix the test in a separate PR.

Try running lifecycle_test.launch (after adding a needed node to it) - works but doesn't result in a non-zero exit code if it does fail (see ros/ros_comm#2082). Turn it into a rostest in separate PR.

lucasw · 2021-09-26T03:31:18Z

Can the requires-changes label be removed if there are no more changes needed?

MichaelGrupp

I left some review comments from my side, because I would also like to get the correct exit code from roslaunch. But I'm not a maintainer.

@sloretz Can you please also check the PR again?

MichaelGrupp · 2021-09-30T14:04:21Z

tools/roslaunch/src/roslaunch/__init__.py

@@ -363,6 +368,8 @@ def main(argv=sys.argv):
            try: os.unlink(options.pid_fn)
            except os.error: pass

+    print('exiting from main() {}'.format(exit_code))


This looks like a debug print that can be removed - people who are interested in the exit code of the executable can get this from the shell.

MichaelGrupp · 2021-09-30T14:05:07Z

tools/roslaunch/src/roslaunch/__init__.py

@@ -313,6 +314,8 @@ def main(argv=sys.argv):
                                               sigint_timeout=options.sigint_timeout,
                                               sigterm_timeout=options.sigterm_timeout)
            c.run()
+            exit_code = c.exit_code
+            logger.warn('child node {} done {}'.format(options.child_name, exit_code))


Why is this a warning? Looks more like a debug thing to me.

MichaelGrupp · 2021-09-30T14:06:00Z

tools/roslaunch/src/roslaunch/__init__.py

@@ -303,7 +304,7 @@ def main(argv=sys.argv):
        logger.info("roslaunch env is %s"%os.environ)

        if options.child_name:
-            logger.info('starting in child mode')
+            logger.info('starting in child mode {}'.format(options.child_name))


Doesn't starting {} in child mode make more sense?

MichaelGrupp · 2021-09-30T14:06:22Z

test/test_roslaunch/test/test_roslaunch_exit_code.py

+import sys
+# import time
+import unittest
+# import yaml


remove unnecessary imports

MichaelGrupp · 2021-09-30T14:07:10Z

test/test_roslaunch/test/test_roslaunch_exit_code.py

+# ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+# POSSIBILITY OF SUCH DAMAGE.
+#
+# Revision $Id: test_roslaunch_command_line_online.py 6411 2009-10-02 21:32:01Z kwc $


this revision string looks like a copied thing that can be removed

MichaelGrupp · 2021-09-30T14:10:42Z

test/test_roslaunch/test/exit_with_code.py

+#!/usr/bin/env python
+# Software License Agreement (BSD License)
+#
+# Copyright (c) 2014, The Johns Hopkins University


The copyright notices look like they were pasted from somewhere else (2014 JHU, 2009 Willow) - do they make sense? I guess here the maintainers need to tell how this should be handled.

Looks like I copied it from test_roslaunch_ros_args.py.

Copyright (c) 2008, Willow Garage, Inc isn't true either, should it be Open Robotics 2021, or my name?

MichaelGrupp · 2021-09-30T14:14:04Z

tools/roslaunch/src/roslaunch/launch.py

@@ -618,7 +619,7 @@ def spin(self):
        #self.pm.join()
        self.logger.info("process monitor is done spinning, initiating full shutdown")
        self.stop()
-        printlog_bold("done")
+        printlog_bold("roslaunch runner done")


I would leave this as it is, it's unrelated to the feature.

MichaelGrupp · 2021-09-30T14:17:41Z

tools/roslaunch/src/roslaunch/pmon.py

@@ -566,6 +567,10 @@ def _run(self):
                        if p.required:
                            printerrlog('='*80+"REQUIRED process [%s] has died!\n%s\nInitiating shutdown!\n"%(p.name, exit_code_str)+'='*80)
                            self.is_shutdown = True
+                            if p.exit_code != 0:
+                                msg = 'Going to exit due to code {}'.format(p.exit_code)


This should also include the p.name of the dead process:

msg = 'Going to exit due to non-zero exit code ({}) of required process {}'.format(p.exit_code, p.name)

MichaelGrupp · 2021-09-30T14:18:43Z

tools/roslaunch/src/roslaunch/__init__.py

@@ -346,6 +349,8 @@ def main(argv=sys.argv):
                    sigterm_timeout=options.sigterm_timeout)
            p.start()
            p.spin()
+            exit_code = p.exit_code
+            print('ending server mode {}'.format(exit_code))


Also here, is this print needed?

… code to the caller...' later

Try running lifecycle_test.launch (after adding a needed node to it) - works but doesn't result in a non-zero exit code if it does fail (see ros/ros_comm#2082). Turn it into a rostest in separate PR.

Resolves ros#919

lucasw force-pushed the exit_code_on_failed branch from 92fad90 to b28da16 Compare October 19, 2020 17:43

lucasw mentioned this pull request Oct 19, 2020

roslaunch exit code 0 even if requried node died with non-zero exit code #919

Open

sloretz requested changes Nov 5, 2020

View reviewed changes

tools/roslaunch/src/roslaunch/__init__.py Outdated Show resolved Hide resolved

tools/roslaunch/src/roslaunch/__init__.py Outdated Show resolved Hide resolved

lucasw force-pushed the exit_code_on_failed branch from 6f45ed9 to 0acad1b Compare November 6, 2020 18:19

jacobperron added enhancement requires-changes labels Dec 10, 2020

Return the exit code to the caller of a required process at least for…

43e5388

… a regular parent launch process, child and remote processes need looking at. May solve ros#919

lucasw force-pushed the exit_code_on_failed branch from 0acad1b to 43e5388 Compare March 21, 2021 14:31

Unit test for roslaunch exit codes

015ac6b

lucasw requested a review from sloretz March 30, 2021 17:34

lucasw added a commit to lucasw/ros1_lifecycle that referenced this pull request May 8, 2021

github actions for 18.04 and 20.04

7f64840

Try running lifecycle_test.launch - fails but doesn't result in a non-zero exit code (see ros/ros_comm#2082). Fix the test in a separate PR.

MichaelGrupp reviewed Sep 30, 2021

View reviewed changes

lucasw added 2 commits September 30, 2021 15:03

Some test cleanup for review comments, squash to test commit later

16b4c18

removed some prints for review comments, squashed to 'Return the exit…

279cdb2

… code to the caller...' later

lucasw force-pushed the exit_code_on_failed branch from e88055b to 279cdb2 Compare September 30, 2021 22:26

rhaschke pushed a commit to ubi-agni/ros_comm that referenced this pull request Aug 20, 2024

roslaunch: return exit code of a required process (ros#2082)

87850c8

Resolves ros#919

rhaschke pushed a commit to ros-o/ros_comm that referenced this pull request Oct 15, 2024

roslaunch: return exit code of a required process (ros#2082)

27027a6

Resolves ros#919

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return the exit code a required process through roslaunch #2082

Return the exit code a required process through roslaunch #2082

lucasw commented Oct 19, 2020

sloretz left a comment

lucasw commented Nov 6, 2020

xkortex commented Feb 1, 2021

LukeAI commented Feb 23, 2021 •

edited

Loading

lucasw commented Mar 21, 2021 •

edited

Loading

lucasw commented Sep 26, 2021

MichaelGrupp left a comment

MichaelGrupp Sep 30, 2021

MichaelGrupp Sep 30, 2021

MichaelGrupp Sep 30, 2021

MichaelGrupp Sep 30, 2021

MichaelGrupp Sep 30, 2021

MichaelGrupp Sep 30, 2021

lucasw Sep 30, 2021

MichaelGrupp Sep 30, 2021

MichaelGrupp Sep 30, 2021

MichaelGrupp Sep 30, 2021

Return the exit code a required process through roslaunch #2082

Are you sure you want to change the base?

Return the exit code a required process through roslaunch #2082

Conversation

lucasw commented Oct 19, 2020

sloretz left a comment

Choose a reason for hiding this comment

lucasw commented Nov 6, 2020

xkortex commented Feb 1, 2021

LukeAI commented Feb 23, 2021 • edited Loading

lucasw commented Mar 21, 2021 • edited Loading

lucasw commented Sep 26, 2021

MichaelGrupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeAI commented Feb 23, 2021 •

edited

Loading

lucasw commented Mar 21, 2021 •

edited

Loading