Autoremove EventHandler from parent entity

[ivanpauno]

Depends on #13.

I can add an equivalent test for subscription events after #14 gets in.

[jacobperron]

Instead of having the parent register a callback, what about instead we call the parent's removeEventHandler() method? This is a pattern I choose when implementing "remove" methods for Node entities like publishers and subscriptions in ros2-java#110; the node removes entities from it's list (and does not dispose them), but if dispose() is called on the entity it removes itself from it's parent.

[ivanpauno]

I didn't follow that approach because of the following problem:

Publisher pub = node.createPublisher(...);
node.dispose();
pub.dispose();  // Use after free here, likely a segfault

My idea is that the above code should work perfectly, pub.dispose() should only be a noop in that case.

In the following example:

Publisher pub = node.createPublisher(...);
node.dispose();
pub.publish();  // I would like an InvalidHandleException thrown here

I would like to see what the inline comment says 😂.

I think that the current implementation of event handlers is solving both problems correctly.
It's a bit annoying to pass the remove callback, but that's an implementation detail that the user doesn't have to know about (and without the callback, there's an infinite recursion problem)

[jacobperron]

Hmm.. it looks like both your examples are handled in the approach I proposed.

If I try:

node.dispose()
pub.dispose()

The program exits cleanly (I believe the pub.dispose() is a noop).

If I try:

node.dispose()
pub.publish()

Then I get the following exception:

Exception in thread "main" org.ros2.rcljava.exceptions.RCLException: Failed to publish: publisher pointer is invalid

I think the exception message is fairly informative, but of course we could change the type like you suggest.

[jacobperron]

We can go forward with the callback approach if you like; I just thought that avoiding the callback registration seemed a bit cleaner.

[ivanpauno]

I have to double check how the approach works, I was almost sure there was an use after free.

[jacobperron]

hmm, I just took another look at the code. I thought we were handling it in Java, but it looks like I may have just got lucky . I'm guessing rcl or rmw is failing gracefully on the publish call. I'm not sure what's going on with the dispose call 🙃. We can leave this PR as-is and I'll have to revisit my approach in ros2-java#110 (with some more unit tests).

[ivanpauno]

node.dispose()
pub.dispose()

There isn't actually an use after free, but the publisher is leaked

ros2_java/rcljava/src/main/cpp/org_ros2_rcljava_publisher_PublisherImpl.cpp

Lines 74 to 77 in 6291847

	if (node_handle == 0) {
	// TODO(esteve): handle this, node is null, but publisher isn't
	return;
	}

.

About:

node.dispose()
pub.publish()

I think that one is UB, as the node has been destroyed and not the publisher (it likely depends on the rmw implementation).

We can leave this PR as-is and I'll have to revisit my approach in ros2-java#110 (with some more unit tests).

👍