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

Add methods to remove entities from Node #110

Merged
merged 3 commits into from
Jun 30, 2020
Merged

Add methods to remove entities from Node #110

merged 3 commits into from
Jun 30, 2020

Conversation

jacobperron
Copy link
Contributor

When an entity is disposed, make sure to also remove it from the Node.
This resolves an issue where invalid entities may be used by other classes or users.
For example, a disposed Subscription being accessed by the executor.

Fixes #105

@esteve esteve self-assigned this Jun 9, 2020
@esteve
Copy link
Member

esteve commented Jun 9, 2020

@jacobperron there seems to be an error in flake8 itself, which causes the tests for rosidl_generator_java to fail. I recall this same issue a while ago was caused by pytest using a non-public API from flake8, either updating ament_flake8 or pinning flake8 to an older version fixed the issue.

2020-06-08T23:30:54.4839662Z 3: Test command: /usr/bin/python3 "-u" "/opt/ros/dashing/share/ament_cmake_test/cmake/run_test.py" "/home/runner/work/ros2_java/ros2_java/ros_ws/build/rosidl_generator_java/test_results/rosidl_generator_java/flake8.xunit.xml" "--package-name" "rosidl_generator_java" "--output-file" "/home/runner/work/ros2_java/ros2_java/ros_ws/build/rosidl_generator_java/ament_flake8/flake8.txt" "--command" "/opt/ros/dashing/bin/ament_flake8" "--xunit-file" "/home/runner/work/ros2_java/ros2_java/ros_ws/build/rosidl_generator_java/test_results/rosidl_generator_java/flake8.xunit.xml"
2020-06-08T23:30:54.4839876Z 3: Test timeout computed to be: 60
2020-06-08T23:30:54.4840274Z 3: -- run_test.py: invoking following command in '/home/runner/work/ros2_java/ros2_java/ros_ws/src/ros2_java/rosidl_generator_java':
2020-06-08T23:30:54.4840738Z 3:  - /opt/ros/dashing/bin/ament_flake8 --xunit-file /home/runner/work/ros2_java/ros2_java/ros_ws/build/rosidl_generator_java/test_results/rosidl_generator_java/flake8.xunit.xml
2020-06-08T23:30:54.4840888Z 3: Traceback (most recent call last):
2020-06-08T23:30:54.4841008Z 3:   File "/opt/ros/dashing/bin/ament_flake8", line 11, in <module>
2020-06-08T23:30:54.4841357Z 3:     load_entry_point('ament-flake8==0.7.11', 'console_scripts', 'ament_flake8')()
2020-06-08T23:30:54.4841699Z 3:   File "/opt/ros/dashing/lib/python3.6/site-packages/ament_flake8/main.py", line 75, in main
2020-06-08T23:30:54.4841827Z 3:     max_line_length=args.linelength)
2020-06-08T23:30:54.4843475Z 3:   File "/opt/ros/dashing/lib/python3.6/site-packages/ament_flake8/main.py", line 163, in generate_flake8_report
2020-06-08T23:30:54.4844163Z 3:     style = get_flake8_style_guide(flake8_argv)
2020-06-08T23:30:54.4845687Z 3:   File "/opt/ros/dashing/lib/python3.6/site-packages/ament_flake8/main.py", line 131, in get_flake8_style_guide
2020-06-08T23:30:54.4846031Z 3:     application.parse_preliminary_options_and_args([])
2020-06-08T23:30:54.4848128Z 3: AttributeError: 'Application' object has no attribute 'parse_preliminary_options_and_args'
2020-06-08T23:30:54.4848509Z 3: -- run_test.py: return code 1
2020-06-08T23:30:54.4848963Z 3: -- run_test.py: generate result file '/home/runner/work/ros2_java/ros2_java/ros_ws/build/rosidl_generator_java/test_results/rosidl_generator_java/flake8.xunit.xml' with failed test
2020-06-08T23:30:54.4849427Z 3: -- run_test.py: verify result file '/home/runner/work/ros2_java/ros2_java/ros_ws/build/rosidl_generator_java/test_results/rosidl_generator_java/flake8.xunit.xml'
2020-06-08T23:30:54.4849571Z 3/6 Test #3: flake8 ..........................................................***Failed    0.55 sec

Edit: never mind, I just saw your message in #109 (comment)

@jacobperron jacobperron mentioned this pull request Jun 10, 2020
@jacobperron
Copy link
Contributor Author

#111 disables the flake8 check for rosidl_generator_java. We can re-enable it for Eloquent/Foxy CI jobs (when we add them), and for Dashing once it is fixed upstream.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One pretty minor thing; otherwise looks good to me.

When an entity is disposed, make sure to also remove it from the Node.
This resolves an issue where invalid entities may be used by other classes or users.
For example, a disposed Subscription being accessed by the executor.

Fixes #105

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Contributor Author

I've rebased to fix CI 🤞

@esteve esteve self-requested a review June 24, 2020 21:54
@jacobperron
Copy link
Contributor Author

@esteve @clalancette PTAL

@esteve esteve merged commit cf2ef07 into dashing Jun 30, 2020
@esteve
Copy link
Member

esteve commented Jun 30, 2020

@jacobperron thanks!

@jacobperron jacobperron deleted the jacob/fix_105 branch June 30, 2020 21:30
jacobperron added a commit that referenced this pull request May 17, 2021
* Add methods to remove entities from Node

When an entity is disposed, make sure to also remove it from the Node.
This resolves an issue where invalid entities may be used by other classes or users.
For example, a disposed Subscription being accessed by the executor.

Fixes #105

Signed-off-by: Jacob Perron <[email protected]>

* Add tests for disposing publishers, services, and clients

Signed-off-by: Jacob Perron <[email protected]>

* Rename test

Signed-off-by: Jacob Perron <[email protected]>
jacobperron added a commit that referenced this pull request May 17, 2021
* Add methods to remove entities from Node

When an entity is disposed, make sure to also remove it from the Node.
This resolves an issue where invalid entities may be used by other classes or users.
For example, a disposed Subscription being accessed by the executor.

Fixes #105

Signed-off-by: Jacob Perron <[email protected]>

* Add tests for disposing publishers, services, and clients

Signed-off-by: Jacob Perron <[email protected]>

* Rename test

Signed-off-by: Jacob Perron <[email protected]>
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