Skip to content

Check that future is done, and always call rclpy.shutdown#273

Merged
sloretz merged 2 commits intomasterfrom
check_future_done
Oct 13, 2021
Merged

Check that future is done, and always call rclpy.shutdown#273
sloretz merged 2 commits intomasterfrom
check_future_done

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Oct 1, 2021

This partially undoes #270 - I didn't realize spin_until_future_complete() does nothing on timeout, so the future's result could be None. This assert's the future is done before using the result.

The second commit Makes sure rclpy.shutdown() is always called. It looks like all the tests are run in the same Python instance, so the tests need to be extra careful when initializing rclpy globally. Using a try/finally construct makes sure rclpy will be shutdown for the next test. Otherwise, errors like this one can happen: https://ci.ros2.org/job/ci_windows/15536/consoleText

===============================================================================================================================
FAIL: set_param_launch_test.TestFixture.test_set_parameter
-------------------------------------------------------------------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\ci\ws\src\ros2\launch_ros\launch_testing_ros\test\examples\set_param_launch_test.py", line 49, in test_set_parameter
    rclpy.init()
  File "C:\ci\ws\install\Lib\site-packages\rclpy\__init__.py", line 76, in init
    return context.init(args, domain_id=domain_id)
  File "C:\ci\ws\install\Lib\site-packages\rclpy\context.py", line 70, in init
    raise RuntimeError('Context.init() must only be called once')
RuntimeError: Context.init() must only be called once
---------------------------- Captured stdout call -----------------------------
[INFO] [launch]: All log files can be found below C:\Users\ContainerAdministrator\.ros\log\2021-09-30-19-48-41-770984-0cec77497c5b-3144
[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [python.exe-3]: process started with pid [872]
[WARNING] [python.exe-3]: 'SIGINT' sent to process[python.exe-3] not supported on Windows, escalating to 'SIGTERM'
[INFO] [python.exe-3]: sending signal 'SIGTERM' to process[python.exe-3]
[ERROR] [python.exe-3]: process has died [pid 872, exit code 1, cmd 'C:\Python38\python.exe C:\ci\ws\src\ros2\launch_ros\launch_testing_ros\test\examples\parameter_blackboard.py --ros-args -r __node:=demo_node_1'].
---------------------------- Captured stderr call -----------------------------
test_set_parameter (set_param_launch_test.TestFixture) ... ERROR

======================================================================
ERROR: test_set_parameter (set_param_launch_test.TestFixture)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\ci\ws\src\ros2\launch_ros\launch_testing_ros\test\examples\set_param_launch_test.py", line 51, in test_set_parameter
    response = node.set_parameter(state=True)
  File "C:\ci\ws\src\ros2\launch_ros\launch_testing_ros\test\examples\set_param_launch_test.py", line 75, in set_parameter
    return response.results[0]
AttributeError: 'NoneType' object has no attribute 'results'

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Oct 1, 2021

CI (build: --packages-up-to launch_testing_ros test: --packages-select launch_testing_ros)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Blast545
Copy link
Copy Markdown
Contributor

More recent CI, --packages-above-and-dependencies launch_testing_ros

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Blast545
Copy link
Copy Markdown
Contributor

As this has been constantly failing in the Windows debug job:

  • Windows Debug Build Status

@Blast545
Copy link
Copy Markdown
Contributor

@sloretz I think it's good to go 👍

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Oct 13, 2021

Thank you @Blast545 for following up on this with CI!

@sloretz sloretz merged commit 4bfd998 into master Oct 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the check_future_done branch October 13, 2021 17:33
@Blast545
Copy link
Copy Markdown
Contributor

👨‍🌾 This extra added assert failed in the windows release nightlies:

assert future.done(), 'Client request timed out'

Can I ask you to take a look @sloretz? 🙏
https://ci.ros2.org/view/nightly/job/nightly_win_rel/2086/

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Oct 14, 2021

@Blast545 That's expected. The service call in the test has been flaky since #263 - this PR improves the error handling so it doesn't affect other tests.

@Blast545
Copy link
Copy Markdown
Contributor

Oh, I see. Thanks for the context!

@Blast545
Copy link
Copy Markdown
Contributor

I don't think it's a flaky call, it has been failing constantly in the past 5 WIndows Release CI jobs. ci.ros2.org/nightly_win_rel#2090/. Can I ask you to take an extra look @sloretz ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants