Thread safe node.destroy_* #319

sloretz · 2019-04-24T15:43:59Z

Follow up to #318 and #330. This PR applies the changes done to Subscription to the rest of the entities on the node so they can all be destroyed safely.

Not done in this PR. These might require a change to the waitable API.

action server
action client

sloretz · 2019-04-25T22:29:12Z

I think I have to include destroy_node in this PR. This approach can change the order that entities get shut down.

# In another thread an executor is spinning
# srv destruction delayed because executor is waiting on service
node.destroy_service(srv)

# node is destroyed instantly; service still not destroyed because it's being used by executor
node.destroy_node()

# When executor shuts down the service is no longer being used, so it's destroyed
# since the node was destroyed first the service will segfault in rmw_fastrtps
executor.shutdown()

I'm thinking of chaining handles so that node.destroy_node() destroy's all node entities before allowing itself to be destroyed.

Publisher now uses Handle pub.publisher_handle -> pub.handle Added test for publisher destruction Added rclpy_publisher_t to replace rcl_publisher_t Signed-off-by: Shane Loretz<sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Client now uses Handle Client.client_handle and client_pointer replaced by client.handle rclpy_destroy_entity no longer destroy's clients Added test for client destruction Client capsule uses rclpy_client_t instead of rcl_client_t Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2019-04-30T20:29:21Z

rclpy/rclpy/executors.py

-        self._guard_condition = gc
-        self._guard_condition_handle = gc_handle
+        self._guard = GuardCondition(
+            callback=None, callback_group=None, context=self._context)


Reusing GuardCondition to reuse executor code that uses guard_condition.handle

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2019-04-30T22:13:34Z

Ready for review.

CI (testing rclpy)

Linux
Linux-aarch64
macOS
Windows

clalancette

Partial review with some feedback on parts I know :). Will do some additional review after.

rclpy/src/rclpy/_rclpy.c

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

clalancette

All right, a few more review comments, mostly phrased as questions as I'm still getting a handle on how this works (pun intended).

rclpy/rclpy/node.py

clalancette · 2019-05-01T00:20:25Z

rclpy/rclpy/node.py

-                return True
+        if publisher in self.publishers:
+            self.publishers.remove(publisher)
+            publisher.destroy()


For all of these destroy calls, do we need to check for the InvalidHandle exception? Is that a case that can happen?

It is. Maybe this should catch it and return False?

Yeah, that seems reasonable.

destroy_<entity>() catch InvalidHandle and return False in e630404

rclpy/rclpy/executors.py

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

clalancette

The code looks good to me now, so I'll approve.

My only remaining question is whether we should open an issue for reducing the complexity here by introducing rmw_shutdown_but_not_fini for a future enhancement. Thoughts on that?

sloretz · 2019-05-01T01:30:14Z

CI (testing rclpy)

Linux
Linux-aarch64
macOS
Windows

sloretz · 2019-05-01T17:56:32Z

My only remaining question is whether we should open an issue for reducing the complexity here by introducing rmw_shutdown_but_not_fini for a future enhancement. Thoughts on that?

I've gone back and forth between writing the issue on rmw vs making it a higher level feature of rcl. I opened ros2/rcl#429, but the solution seems a lot more complicated than I first thought. I put it in the backlog since it feels like it needs more thought.

sloretz added the in progress Actively being worked on (Kanban column) label Apr 24, 2019

sloretz self-assigned this Apr 24, 2019

sloretz force-pushed the sloretz/thread_safe_destroy branch from 7ba0972 to 0e8cda0 Compare April 25, 2019 22:17

sloretz mentioned this pull request Apr 26, 2019

Wait set uses pointers to rcl types not rcl->impl types #324

Merged

sloretz force-pushed the sloretz/thread_safe_destroy branch from c6e3e8c to 3ac25ce Compare April 26, 2019 21:08

This was referenced Apr 29, 2019

Don't use multiprocessing in tests #328

Merged

Make destroy_node thread safe #330

Merged

sloretz force-pushed the sloretz/thread_safe_destroy branch from 3ac25ce to 5bea9ba Compare April 30, 2019 00:25

sloretz mentioned this pull request Apr 30, 2019

QoS - API and implementation for Liveliness and Deadline event callbacks #316

Merged

sloretz added 12 commits April 30, 2019 13:26

Remove unnecessary comment

3d7857e

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Publisher rclpy_publisher_t needs valid node

8832f9d

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

BROKEN: node.destroy_service thread safe

02a3b85

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Shutdown executor so service is destroyed before rclpy

68c80c8

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Timer uses Handle

cd4e5ed

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Guard conditions use Handle

6f28e8e

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Tests pass again after rebasing

e7310a2

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

rcl_clock_t capsule uses Handle

197431b

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

More checks in destructor

bbe6572

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Uncrustify fixes

e4ca965

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz force-pushed the sloretz/thread_safe_destroy branch from 052b0ed to e4ca965 Compare April 30, 2019 20:26

sloretz commented Apr 30, 2019

View reviewed changes

Undo changes to test_action_client

ff0b30c

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 30, 2019

clalancette reviewed Apr 30, 2019

View reviewed changes

rclpy/src/rclpy/_rclpy.c Show resolved Hide resolved

rclpy/src/rclpy/_rclpy.c Show resolved Hide resolved

check pointers before using them

5f7b7c8

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz mentioned this pull request May 1, 2019

Replace NULL == foo with !foo #331

Closed

clalancette reviewed May 1, 2019

View reviewed changes

sloretz added 2 commits April 30, 2019 18:00

destroy_<entity> catches InvalidHandle

e630404

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Clear lists rather than changing type

4bcfe99

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

clalancette approved these changes May 1, 2019

View reviewed changes

sloretz mentioned this pull request May 1, 2019

Add rcl_..._shutdown ros2/rcl#429

Open

sloretz merged commit d672075 into master May 1, 2019

delete-merged-branch bot deleted the sloretz/thread_safe_destroy branch May 1, 2019 17:56

sloretz removed the in review Waiting for review (Kanban column) label May 1, 2019

This was referenced May 2, 2019

Remove extra references to node handle #335

Merged

occasional segfault in shutdown, perhaps in node.destroy_node() #178

Closed

QoS - Expose the assert_liveliness API for Publishers and Nodes #313

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread safe node.destroy_* #319

Thread safe node.destroy_* #319

sloretz commented Apr 24, 2019 •

edited

Loading

sloretz commented Apr 25, 2019

sloretz Apr 30, 2019

sloretz commented Apr 30, 2019

clalancette left a comment

clalancette left a comment

clalancette May 1, 2019

sloretz May 1, 2019

clalancette May 1, 2019

sloretz May 1, 2019

clalancette left a comment

sloretz commented May 1, 2019

sloretz commented May 1, 2019

Thread safe node.destroy_* #319

Thread safe node.destroy_* #319

Conversation

sloretz commented Apr 24, 2019 • edited Loading

sloretz commented Apr 25, 2019

sloretz Apr 30, 2019

Choose a reason for hiding this comment

sloretz commented Apr 30, 2019

clalancette left a comment

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

clalancette May 1, 2019

Choose a reason for hiding this comment

sloretz May 1, 2019

Choose a reason for hiding this comment

clalancette May 1, 2019

Choose a reason for hiding this comment

sloretz May 1, 2019

Choose a reason for hiding this comment

clalancette left a comment

Choose a reason for hiding this comment

sloretz commented May 1, 2019

sloretz commented May 1, 2019

sloretz commented Apr 24, 2019 •

edited

Loading