From b16be6c8f7f384c6d44dcb2564b38c4eb71e3329 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 8 Jun 2020 16:13:32 -0700 Subject: [PATCH 1/3] 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 --- .../org/ros2/rcljava/client/ClientImpl.java | 1 + .../main/java/org/ros2/rcljava/node/Node.java | 49 +++++++++++++++++++ .../java/org/ros2/rcljava/node/NodeImpl.java | 28 +++++++++++ .../ros2/rcljava/publisher/PublisherImpl.java | 1 + .../org/ros2/rcljava/service/ServiceImpl.java | 1 + .../subscription/SubscriptionImpl.java | 1 + .../subscription/SubscriptionTest.java | 10 +++- 7 files changed, 90 insertions(+), 1 deletion(-) diff --git a/rcljava/src/main/java/org/ros2/rcljava/client/ClientImpl.java b/rcljava/src/main/java/org/ros2/rcljava/client/ClientImpl.java index 7b2a8557..076b99cc 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/client/ClientImpl.java +++ b/rcljava/src/main/java/org/ros2/rcljava/client/ClientImpl.java @@ -131,6 +131,7 @@ public final Class getResponseType() { public final void dispose() { Node node = this.nodeReference.get(); if (node != null) { + node.removeClient(this); nativeDispose(node.getHandle(), this.handle); this.handle = 0; } diff --git a/rcljava/src/main/java/org/ros2/rcljava/node/Node.java b/rcljava/src/main/java/org/ros2/rcljava/node/Node.java index 39dab4b2..0a3ba7ad 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/node/Node.java +++ b/rcljava/src/main/java/org/ros2/rcljava/node/Node.java @@ -122,6 +122,55 @@ Client createClient( Client createClient(final Class serviceType, final String serviceName) throws NoSuchFieldException, IllegalAccessException; + /** + * Remove a Subscription created by this Node. + * + * Calling this method effectively invalidates the passed @{link Subscription}. + * If the subscription was not created by this Node, then nothing happens. + * + * @param subscription The object to remove from this node. + * @return true if the subscription was removed, false if the subscription was already + * removed or was never created by this Node. + */ + boolean removeSubscription(final Subscription subscription); + + /** + * Remove a Publisher created by this Node. + * + * Calling this method effectively invalidates the passed @{link Publisher}. + * If the publisher was not created by this Node, then nothing happens. + * + * @param publisher The object to remove from this node. + * @return true if the publisher was removed, false if the publisher was already + * removed or was never created by this Node. + */ + boolean removePublisher(final Publisher publisher); + + /** + * Remove a Service created by this Node. + * + * Calling this method effectively invalidates the passed @{link Service}. + * If the service was not created by this Node, then nothing happens. + * + * @param service The object to remove from this node. + * @return true if the service was removed, false if the service was already + * removed or was never created by this Node. + */ + boolean removeService(final Service service); + + /** + * Remove a Client created by this Node. + * + * Calling this method effectively invalidates the passed @{link Client}. + * If the client was not created by this Node, then nothing happens. + * + * @param client The object to remove from this node. + * @return true if the client was removed, false if the client was already + * removed or was never created by this Node. + */ + boolean removeClient(final Client client); + + WallTimer createWallTimer(final long period, final TimeUnit unit, final Callback callback); String getName(); diff --git a/rcljava/src/main/java/org/ros2/rcljava/node/NodeImpl.java b/rcljava/src/main/java/org/ros2/rcljava/node/NodeImpl.java index 2235fda0..68018fa8 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/node/NodeImpl.java +++ b/rcljava/src/main/java/org/ros2/rcljava/node/NodeImpl.java @@ -221,6 +221,20 @@ public final Subscription createSubscription( return this.createSubscription(messageType, topic, callback, QoSProfile.DEFAULT); } + /** + * {@inheritDoc} + */ + public boolean removeSubscription(final Subscription subscription) { + return this.subscriptions.remove(subscription); + } + + /** + * {@inheritDoc} + */ + public boolean removePublisher(final Publisher publisher) { + return this.publishers.remove(publisher); + } + /** * {@inheritDoc} */ @@ -300,6 +314,20 @@ public Client createClient(final Class servi private static native long nativeCreateClientHandle( long handle, Class cls, String serviceName, long qosProfileHandle); + /** + * {@inheritDoc} + */ + public boolean removeService(final Service service) { + return this.services.remove(service); + } + + /** + * {@inheritDoc} + */ + public boolean removeClient(final Client client) { + return this.clients.remove(client); + } + /** * {@inheritDoc} */ diff --git a/rcljava/src/main/java/org/ros2/rcljava/publisher/PublisherImpl.java b/rcljava/src/main/java/org/ros2/rcljava/publisher/PublisherImpl.java index f0b3cb6b..f3fa717b 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/publisher/PublisherImpl.java +++ b/rcljava/src/main/java/org/ros2/rcljava/publisher/PublisherImpl.java @@ -117,6 +117,7 @@ public final WeakReference getNodeReference() { public final void dispose() { Node node = this.nodeReference.get(); if (node != null) { + node.removePublisher(this); nativeDispose(node.getHandle(), this.handle); this.handle = 0; } diff --git a/rcljava/src/main/java/org/ros2/rcljava/service/ServiceImpl.java b/rcljava/src/main/java/org/ros2/rcljava/service/ServiceImpl.java index a438e72b..e45d06ec 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/service/ServiceImpl.java +++ b/rcljava/src/main/java/org/ros2/rcljava/service/ServiceImpl.java @@ -85,6 +85,7 @@ public final Class getResponseType() { public final void dispose() { Node node = this.nodeReference.get(); if (node != null) { + node.removeService(this); nativeDispose(node.getHandle(), this.handle); this.handle = 0; } diff --git a/rcljava/src/main/java/org/ros2/rcljava/subscription/SubscriptionImpl.java b/rcljava/src/main/java/org/ros2/rcljava/subscription/SubscriptionImpl.java index 1cdd48cf..47fce59a 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/subscription/SubscriptionImpl.java +++ b/rcljava/src/main/java/org/ros2/rcljava/subscription/SubscriptionImpl.java @@ -129,6 +129,7 @@ public final WeakReference getNodeReference() { public final void dispose() { Node node = this.nodeReference.get(); if (node != null) { + node.removeSubscription(this); nativeDispose(node.getHandle(), this.handle); this.handle = 0; } diff --git a/rcljava/src/test/java/org/ros2/rcljava/subscription/SubscriptionTest.java b/rcljava/src/test/java/org/ros2/rcljava/subscription/SubscriptionTest.java index ee23ff47..8c990480 100644 --- a/rcljava/src/test/java/org/ros2/rcljava/subscription/SubscriptionTest.java +++ b/rcljava/src/test/java/org/ros2/rcljava/subscription/SubscriptionTest.java @@ -26,7 +26,7 @@ public class SubscriptionTest { @Test - public final void testCreate() { + public final void testCreateAndDispose() { RCLJava.rclJavaInit(); Node node = RCLJava.createNode("test_node"); Subscription subscription = node.createSubscription( @@ -36,6 +36,14 @@ public void accept(final std_msgs.msg.String msg) {} assertEquals(node.getHandle(), subscription.getNodeReference().get().getHandle()); assertNotEquals(0, subscription.getNodeReference().get().getHandle()); assertNotEquals(0, subscription.getHandle()); + assertEquals(1, node.getSubscriptions().size()); + + // We expect that calling dispose should result in a zero handle + // and the reference is dropped from the Node + subscription.dispose(); + assertEquals(0, subscription.getHandle()); + assertEquals(0, node.getSubscriptions().size()); + RCLJava.shutdown(); } } From 085fc24ac1ce3a7c6cdbff716cd78abfb68868bb Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Wed, 24 Jun 2020 14:05:10 -0700 Subject: [PATCH 2/3] Add tests for disposing publishers, services, and clients Signed-off-by: Jacob Perron --- .../src/test/java/org/ros2/rcljava/client/ClientTest.java | 8 +++++++- .../java/org/ros2/rcljava/publisher/PublisherTest.java | 8 ++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/rcljava/src/test/java/org/ros2/rcljava/client/ClientTest.java b/rcljava/src/test/java/org/ros2/rcljava/client/ClientTest.java index df7d2a6c..f4724d9b 100644 --- a/rcljava/src/test/java/org/ros2/rcljava/client/ClientTest.java +++ b/rcljava/src/test/java/org/ros2/rcljava/client/ClientTest.java @@ -111,10 +111,16 @@ public final void testAdd() throws Exception { // Check the contents of the response assertEquals(5, response.getSum()); - // Cleanup + assertEquals(1, node.getClients().size()); + assertEquals(1, node.getServices().size()); + + // We expect that calling dispose should result in a zero handle + // and the reference is dropped from the Node client.dispose(); assertEquals(0, client.getHandle()); + assertEquals(0, node.getClients().size()); service.dispose(); assertEquals(0, service.getHandle()); + assertEquals(0, node.getServices().size()); } } diff --git a/rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java b/rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java index bfcba4e0..e130ae2c 100644 --- a/rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java +++ b/rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java @@ -33,6 +33,14 @@ public final void testCreate() { assertEquals(node.getHandle(), publisher.getNodeReference().get().getHandle()); assertNotEquals(0, publisher.getNodeReference().get().getHandle()); assertNotEquals(0, publisher.getHandle()); + assertEquals(1, node.getPublishers().size()); + + // We expect that calling dispose should result in a zero handle + // and the reference is dropped from the Node + publisher.dispose(); + assertEquals(0, publisher.getHandle()); + assertEquals(0, node.getPublishers().size()); + RCLJava.shutdown(); } } From c8e5d854181ad9157311946ff1ae751d93da0e0e Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Wed, 24 Jun 2020 14:10:06 -0700 Subject: [PATCH 3/3] Rename test Signed-off-by: Jacob Perron --- .../src/test/java/org/ros2/rcljava/publisher/PublisherTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java b/rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java index e130ae2c..2ce379fd 100644 --- a/rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java +++ b/rcljava/src/test/java/org/ros2/rcljava/publisher/PublisherTest.java @@ -25,7 +25,7 @@ public class PublisherTest { @Test - public final void testCreate() { + public final void testCreateAndDispose() { RCLJava.rclJavaInit(); Node node = RCLJava.createNode("test_node"); Publisher publisher =