Fix object destruction order #497

ivanpauno · 2020-01-20T17:09:11Z

Supersedes #470.
See #470 (comment).

Solves strange bugs related with destruction order.

Included in this PR:

rclpy_handle_t struct, related methods, and usage of it in Handle class.
Replaces usage of PyCapsule for rclpy_handle_t where needed.
Some formatting changes (avoid unnecessary castings, etc).

Not included (subject of a follow up):

rmw_request_id_t capsule is leaking.
rmw_qos_profile_t capsule is leaking.
Refactor wait set related code, so it can be wrapped by a Handle.
Refactor rclpy_action objects, so they can be wrapped in a Handle.

ivanpauno · 2020-01-20T17:12:39Z

CI above rclpy:

Linux

ivanpauno · 2020-01-21T15:54:47Z

New CI, after fixing failures:

Linux

rclpy/CMakeLists.txt

rclpy/rclpy/handle.py

rclpy/src/rclpy/_rclpy.c

hidmic · 2020-01-27T16:02:44Z

rclpy/src/rclpy/_rclpy.c

@@ -2766,7 +2744,7 @@ rclpy_wait(PyObject * Py_UNUSED(self), PyObject * args)
  if (!PyArg_ParseTuple(args, "O|K", &pywait_set, &timeout)) {
    return NULL;
  }
-  rcl_wait_set_t * wait_set = (rcl_wait_set_t *)PyCapsule_GetPointer(pywait_set, "rcl_wait_set_t");
+  rcl_wait_set_t * wait_set = PyCapsule_GetPointer(pywait_set, "rcl_wait_set_t");


@ivanpauno why doesn't rcl_wait_set_t follow the same treatment that other rcl types' allocations do i.e. using rclpy_handle_t too?

Good question. Currently, the rcl_wait_set_t capsule didn't have a deleter, and deletion was handled manually in Python.
To avoid doing this PR bigger, I didn't refactor how wait sets were used. It should be done in a follow up PR.

Let's open an issue to track that work.

rclpy/src/rclpy_common/include/rclpy_common/handle.h

rclpy/src/rclpy_common/src/handle.c

rclpy/src/test/rclpy_common/test_handle.cpp

hidmic

LGTM but we should follow up on leaking resources ASAP

rclpy/src/test/rclpy_common/test_handle.cpp

sloretz

Partial review. Will continue later.

rclpy/rclpy/handle.py

rclpy/src/rclpy_common/src/handle.c

sloretz · 2020-01-28T20:30:35Z

rclpy/rclpy/handle.py

+        The managed `rclpy_handle_t` object will not be destructed immediately if another handle
+        called `other.requires(this)`.
+        In that case, the managed object will be destroyed after all its
+        dependents are destroyed.


Can Handle call destroy() on all the downstream handles too? Without that, there must be something else that has duplicate information of which handle requires what so they can be destroyed in the right order. If I want to destroy Node, I must destroy the publishers, subscribers, etc created from it first. In practice this responsibility is owned by Node.destroy_node(), but that only works because Node also knows which handles require its handle.

That duplicate knowledge is hard to do for Context. Without that knowledge duplicated it means rclpy.shutdown() doesn't actually shutdown the library; instead everything must wait for the garbage collector.

That duplicate knowledge is hard to do for Context. Without that knowledge duplicated it means rclpy.shutdown() doesn't actually shutdown the library; instead everything must wait for the garbage collector.

Up to now, shutting down rclpy wasn't destructing all the rcl created objects, as Context wasn't using a Handle. If we want that feature, I would rather do it in a follow-up PR, and limit this one to fix the order destruction problem (it can be done by keeping track of all the nodes created by a context).

There's also some inconsistency of what happens when a node is destructed:

Publishers, Subscriptions, Clients, Services, Timers, Guards created by a node are destructed.

Action clients and action servers aren't.

We can also rely on things being explicitly destroyed in reversed order and fail if destruction of an object other objects still depend on is attempted.

If I may, I have to ask, where does this need of explicitly destroying objects come from? It's somewhat incompatible with Python, it creates invalid objects, it makes resource management more complex and it could be easily circumvented by calling gc.collect if need be. It almost seems to me like we're fighting the language to do things in a way it was not meant to.

If I may, I have to ask, where does this need of explicitly destroying objects come from? It's somewhat incompatible with Python, it creates invalid objects, it makes resource management more complex and it could be easily circumvented by calling gc.collect if need be. It almost seems to me like we're fighting the language to do things in a way it was not meant to.

I hear you. This is an old discussion. I've been on that side before, and I'm still mostly on that side. I agree we don't need to free memory right away. There's nothing special about our ram usage vs python's, so if the python garbage collector is good enough for some then it is good enough for all.

The reason to destroy these objects is not about their memory usage, it's about freeing system resources. Explicitly freeing system resources without waiting for the garbage collector is not fighting the language; it's pretty normal.

https://docs.python.org/3/library/socket.html#socket.close

https://docs.python.org/3/library/io.html#io.IOBase.close

https://docs.python.org/3/library/subprocess.html#subprocess.Popen.kill

Maybe it could be circumvented by calling gc.collect(), but that's not how python objects usually solve this situation: https://docs.python.org/3/tutorial/inputoutput.html .

If you’re not using the with keyword, then you should call f.close() to close the file and immediately free up any system resources used by it. If you don’t explicitly close a file, Python’s garbage collector will eventually destroy the object and close the open file for you, but the file may stay open for a while. Another risk is that different Python implementations will do this clean-up at different times.

Yes rcl objects have to be destroyed in order, but that has nothing to do with why we want to destroy them. We want to destroy them because that's the only way the rcl API gives us to free system resources they use. Depending on the rmw implementation, nodes/pubs/subs/etc open sockets, they're using network bandwidth by continuing to participate in discovery, subscribers are receiving data from other publishers on the network, service servers may be accepting requests even if the intent is for another server to take their place, etc. That's where the need of explicitly destroying objects comes from.

I understand the point of freeing resources. But, my point is that rclpy.shutdown is currently not doing so. I think that the responsability of doing that should be of the Context itself, and not of Handle. I can add that, but IMO it's subject of a follow up.

my point is that rclpy.shutdown is currently not doing so. I think that the responsability of doing that should be of the Context itself, and not of Handle. I can add that, but IMO it's subject of a follow up.

That's a fair point. I think I'm getting hung up on what I intended Handle to do vs the reality of that never being fully implemented. I do like the approach this PR takes, and I think it makes it pretty easy to get to a state of everything being cleaned up right away.

If h1.requires(h0) caused h0 to store a weakref to h1, then h0.destory() could call h1.destroy() without worrying about destruction order of h1's dependents because the reference counting in this PR would take care of that. I think doing that is important for the reasons above, but you're right it doesn't need to be done right away if the feature doesn't fully exist now. I'll dismiss my review since it seems to be blocking progress. Sorry about that.

Can be done in follow up PR

rclpy/rclpy/handle.py

rclpy/src/rclpy/_rclpy.c

rclpy/src/rclpy/_rclpy_qos_event.c

rclpy/src/rclpy_common/include/rclpy_common/handle.h

ivanpauno · 2020-02-17T14:42:08Z

Linux

ivanpauno · 2020-02-20T16:54:03Z

@hidmic About the regression test you asked, I gave it a try, but it's super hard to reproduce the problem.
The only example of the problem I found is #470, which is not more reproducible after #488 and #490.

My main reason to move lifetime manangement to C is that destruction order is not documented in Python, nor how the gc works. Both of them are an implementation detail of a particular python interpreter, and users shouldn't make any supposition about how they work.

From PEP 442:

Cyclic isolate (CI)
A standalone subgraph of objects in which no object is referenced from the outside, containing one or several reference cycles, and whose objects are still in a usable, non-broken state: they can access each other from their respective finalizers.

For CI objects, the order in which finalizers are called (step 2 above) is undefined.

hidmic · 2020-02-27T18:34:43Z

I think listing that PEP to back up rclpy_handler_t rationale would be great for our future selves.

hidmic

LGTM but for a question, though some green CI would be nice too

rclpy/src/rclpy/_rclpy.c

rclpy/src/rclpy_common/include/rclpy_common/handle.h

hidmic · 2020-02-27T18:54:11Z

rclpy/test/test_handle.py

-        with node1.handle:
-            pass
-    finally:
-        rclpy.shutdown(context=context)


@ivanpauno why were these tests dropped? they certainly have to be rewritten but still apply conceptually, no?

Those tests were relaying in using requires, which is not exposed more.
I think that the tests I wrote in https://github.com/ros2/rclpy/blob/39a93ac5cde247b53137441e34f431d68ba211c4/rclpy/src/test/rclpy_common/test_handle.cpp are a good replacement.

I see, fair enough.

Signed-off-by: Ivan Santiago Paunovic <[email protected]> Add node handle dependeny on context handle Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno · 2020-02-27T21:04:11Z

I think listing that PEP to back up rclpy_handler_t rationale would be great for our future selves.

See 826e6cc.

hidmic

LGTM again but for a few nits, and pending green CI

rclpy/src/rclpy_common/include/rclpy_common/handle.h

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno · 2020-03-02T15:35:32Z

Linux (unrelated failures)
Linux-aarch64 (unrelated failures)
macOS
Windows (unrelated failures)

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno · 2020-03-03T13:59:03Z

macOS (unrelated failures)

hidmic

LGTM !

ivanpauno requested review from dirk-thomas, sloretz and hidmic January 20, 2020 17:09

ivanpauno self-assigned this Jan 20, 2020

ivanpauno mentioned this pull request Jan 20, 2020

Context destruction order bug #470

Closed

ivanpauno added the bug Something isn't working label Jan 21, 2020

hidmic reviewed Jan 27, 2020

View reviewed changes

ivanpauno requested a review from hidmic January 28, 2020 17:56

hidmic approved these changes Jan 28, 2020

View reviewed changes

rclpy/src/test/rclpy_common/test_handle.cpp Outdated Show resolved Hide resolved

rclpy/src/test/rclpy_common/test_handle.cpp Outdated Show resolved Hide resolved

sloretz previously requested changes Jan 28, 2020

View reviewed changes

ivanpauno force-pushed the ivanpauno/fix-destruction-order-2 branch from 267e393 to cd37c5a Compare February 12, 2020 19:54

ivanpauno requested a review from hidmic February 12, 2020 19:55

hidmic reviewed Feb 13, 2020

View reviewed changes

ivanpauno force-pushed the ivanpauno/fix-destruction-order-2 branch from cd37c5a to 034da37 Compare February 14, 2020 13:16

ivanpauno requested a review from hidmic February 14, 2020 13:39

ivanpauno force-pushed the ivanpauno/fix-destruction-order-2 branch from c0c56b9 to b08ad1b Compare February 17, 2020 14:40

hidmic approved these changes Feb 27, 2020

View reviewed changes

ivanpauno added 7 commits February 27, 2020 16:43

Ensure correct destruction order by managing lifetimes in C.

4b15bc9

Signed-off-by: Ivan Santiago Paunovic <[email protected]> Add node handle dependeny on context handle Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Move lifetime management completly to C

3af79bb

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Improve documentation

e281830

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Reuse destroy functions

5ae625d

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Fix bad error handling

225454c

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Add rationale

826e6cc

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Correct typos

a0f8f3d

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Please linters

39a93ac

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno force-pushed the ivanpauno/fix-destruction-order-2 branch from b08ad1b to 39a93ac Compare February 27, 2020 21:03

ivanpauno requested a review from hidmic February 27, 2020 21:06

hidmic approved these changes Mar 2, 2020

View reviewed changes

Improve comments

269486e

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno added 2 commits March 2, 2020 14:53

Make windows visibility happy

8e6672d

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Add CXX standard to avoid macOS failures

9764477

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno requested review from sloretz and hidmic and removed request for dirk-thomas and sloretz March 3, 2020 13:59

hidmic approved these changes Mar 4, 2020

View reviewed changes

ivanpauno merged commit 131a91d into master Mar 4, 2020

delete-merged-branch bot deleted the ivanpauno/fix-destruction-order-2 branch March 4, 2020 15:55

ivanpauno mentioned this pull request Mar 5, 2020

Ensure logging is initialized only once #518

Merged

dirk-thomas mentioned this pull request Jul 9, 2020

Regression on expected number of event handlers in test_qos_event #588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix object destruction order #497

Fix object destruction order #497

ivanpauno commented Jan 20, 2020

ivanpauno commented Jan 20, 2020

ivanpauno commented Jan 21, 2020

hidmic Jan 27, 2020

ivanpauno Jan 27, 2020

hidmic Jan 28, 2020

hidmic left a comment

sloretz left a comment

sloretz Jan 28, 2020

ivanpauno Jan 29, 2020

hidmic Jan 29, 2020

sloretz Jan 29, 2020

ivanpauno Jan 30, 2020

sloretz Feb 3, 2020

ivanpauno commented Feb 17, 2020

ivanpauno commented Feb 20, 2020

hidmic commented Feb 27, 2020

hidmic left a comment

hidmic Feb 27, 2020

ivanpauno Feb 27, 2020

hidmic Mar 2, 2020

ivanpauno commented Feb 27, 2020

hidmic left a comment

ivanpauno commented Mar 2, 2020 •

edited

Loading

ivanpauno commented Mar 3, 2020 •

edited

Loading

hidmic left a comment

Fix object destruction order #497

Fix object destruction order #497

Conversation

ivanpauno commented Jan 20, 2020

ivanpauno commented Jan 20, 2020

ivanpauno commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

sloretz 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

ivanpauno commented Feb 17, 2020

ivanpauno commented Feb 20, 2020

hidmic commented Feb 27, 2020

hidmic 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

ivanpauno commented Feb 27, 2020

hidmic left a comment

Choose a reason for hiding this comment

ivanpauno commented Mar 2, 2020 • edited Loading

ivanpauno commented Mar 3, 2020 • edited Loading

hidmic left a comment

Choose a reason for hiding this comment

ivanpauno commented Mar 2, 2020 •

edited

Loading

ivanpauno commented Mar 3, 2020 •

edited

Loading