Skip to content

Autoremove EventHandler from parent entity #16

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 26, 2020
Merged

Autoremove EventHandler from parent entity #16

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb403a3
Autoremove EventHandler from parent
ivanpauno Aug 20, 2020
a0335cc
Remove duplicated import
ivanpauno Aug 26, 2020
f1c70c3
Rename removeCallback as disposeCallback
ivanpauno Aug 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions rcljava/src/main/java/org/ros2/rcljava/events/EventHandlerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,19 @@ public class EventHandlerImpl<
* @param eventStatusFactory Factory of an event status.
* @param callback The callback function that will be called when the event
* is triggered.
* @param removeCallback Callback that will be called when this object is being disposed.
*/
public EventHandlerImpl(
final WeakReference<ParentT> parentReference,
final long handle,
final Supplier<T> eventStatusFactory,
final Consumer<T> callback) {
final Consumer<T> callback,
final Consumer<EventHandler> removeCallback) {
this.parentReference = parentReference;
this.handle = handle;
this.eventStatusFactory = eventStatusFactory;
this.callback = callback;
this.removeCallback = removeCallback;
}

/**
Expand Down Expand Up @@ -102,9 +105,11 @@ public final long getHandle() {
* {@inheritDoc}
*/
public synchronized final void dispose() {
if (this.handle != 0) {
nativeDispose(this.handle);
if (this.handle == 0) {
return;
}
this.removeCallback.accept(this);
Copy link

@jacobperron jacobperron Aug 26, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@ivanpauno ivanpauno Aug 26, 2020

Choose a reason for hiding this comment

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

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)

Copy link

@jacobperron jacobperron Aug 26, 2020

Choose a reason for hiding this comment

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

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.

Copy link

@jacobperron jacobperron Aug 26, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@ivanpauno ivanpauno Aug 26, 2020

Choose a reason for hiding this comment

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

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

Copy link

@jacobperron jacobperron Aug 26, 2020

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

@ivanpauno ivanpauno Aug 26, 2020

Choose a reason for hiding this comment

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

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).

👍

nativeDispose(this.handle);
this.handle = 0;
}

Expand All @@ -126,11 +131,12 @@ public synchronized final void executeCallback() {
nativeTake(this.handle, nativeEventStatusHandle);
eventStatus.fromRCLEvent(nativeEventStatusHandle);
eventStatus.deallocateRCLStatusEvent(nativeEventStatusHandle);
callback.accept(eventStatus);
this.callback.accept(eventStatus);
}

private final Supplier<T> eventStatusFactory;
private final WeakReference<ParentT> parentReference;
private long handle;
private final Consumer<T> callback;
private final Consumer<EventHandler> removeCallback;
}
12 changes: 11 additions & 1 deletion rcljava/src/main/java/org/ros2/rcljava/publisher/PublisherImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,20 @@ public final WeakReference<Node> getNodeReference() {
public final
<T extends PublisherEventStatus> EventHandler<T, Publisher>
createEventHandler(Supplier<T> factory, Consumer<T> callback) {
final WeakReference<Collection<EventHandler>> weakEventHandlers = new WeakReference(
this.eventHandlers);
Consumer<EventHandler> removeCallback = new Consumer<EventHandler>() {
public void accept(EventHandler eventHandler) {
Collection<EventHandler> eventHandlers = weakEventHandlers.get();
if (eventHandlers != null) {
eventHandlers.remove(eventHandler);
}
}
};
T status = factory.get();
long eventHandle = nativeCreateEvent(this.handle, status.getPublisherEventType());
EventHandler<T, Publisher> eventHandler = new EventHandlerImpl(
new WeakReference<Publisher>(this), eventHandle, factory, callback);
new WeakReference<Publisher>(this), eventHandle, factory, callback, removeCallback);
this.eventHandlers.add(eventHandler);
return eventHandler;
}
Expand Down
13 changes: 12 additions & 1 deletion rcljava/src/main/java/org/ros2/rcljava/subscription/SubscriptionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import org.ros2.rcljava.RCLJava;
import org.ros2.rcljava.common.JNIUtils;
import org.ros2.rcljava.consumers.BiConsumer;
import org.ros2.rcljava.consumers.Consumer;
import org.ros2.rcljava.events.EventHandler;
import org.ros2.rcljava.events.EventHandlerImpl;
Expand Down Expand Up @@ -128,10 +129,20 @@ public final WeakReference<Node> getNodeReference() {
public final
<T extends SubscriptionEventStatus> EventHandler<T, Subscription>
createEventHandler(Supplier<T> factory, Consumer<T> callback) {
final WeakReference<Collection<EventHandler>> weakEventHandlers = new WeakReference(
this.eventHandlers);
Consumer<EventHandler> removeCallback = new Consumer<EventHandler>() {
public void accept(EventHandler eventHandler) {
Collection<EventHandler> eventHandlers = weakEventHandlers.get();
if (eventHandlers != null) {
eventHandlers.remove(eventHandler);
}
}
};
T status = factory.get();
long eventHandle = nativeCreateEvent(this.handle, status.getSubscriptionEventType());
EventHandler<T, Subscription> eventHandler = new EventHandlerImpl(
new WeakReference<Subscription>(this), eventHandle, factory, callback);
new WeakReference<Subscription>(this), eventHandle, factory, callback, removeCallback);
this.eventHandlers.add(eventHandler);
return eventHandler;
}
Expand Down
5 changes: 4 additions & 1 deletion rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,12 @@ public void accept(final OfferedQosIncompatible status) {
}
);
assertNotEquals(0, eventHandler.getHandle());
assertEquals(1, publisher.getEventHandlers().size());
// force executing the callback, so we check that taking an event works
eventHandler.executeCallback();
RCLJava.shutdown();
eventHandler.dispose();
assertEquals(0, eventHandler.getHandle());
assertEquals(0, publisher.getEventHandlers().size());
RCLJava.shutdown();
}
}