Improve resiliency of entities when destroyed

lib/action/client.js

@@ -363,14 +363,13 @@ class ActionClient extends Entity {
    * @return {undefined}
    */
   destroy() {
-    if (this._destroyed) {
+    if (this.isDestroyed()) {
       return;
     }
 
     this._goalHandles.clear();
 
     this._node._destroyEntity(this, this._node._actionClients);
-    this._destroyed = true;
   }
 }
 

lib/action/server.js

@@ -440,7 +440,7 @@ class ActionServer extends Entity {
    * @return {undefined}
    */
   destroy() {
-    if (this._destroyed) {
+    if (this.isDestroyed()) {
       return;
     }
 
@@ -451,7 +451,6 @@ class ActionServer extends Entity {
     this._goalHandles.clear();
 
     this._node._destroyEntity(this, this._node._actionServers);
-    this._destroyed = true;
   }
 }
 

lib/entity.js

@@ -14,16 +14,36 @@
 
 'use strict';
 
+// Destroying an entity for which there is immediate i/o activity can
+// results in a SEGFAULT. To mitigate this situation we hold a
+// reference to released handles to prevent them from being
+// GC'ed prematurely.
+const OBSOLETE_HANDLES = new Set();
+function registerObsoleteHandle(handle) {
+  OBSOLETE_HANDLES.add(handle);
+}
+
 /**
  * @class - Class representing a common object in RCL.
  * @ignore
  */
 
 class Entity {
+  /**
+   * Clears the internal short-lived cache of references to
+   * destroyed entities.
+   *
+   * @ignore
+   */
+  static _gcHandles() {
+    OBSOLETE_HANDLES.clear();
+  }
+
   constructor(handle, typeClass, options) {
     this._handle = handle;
     this._typeClass = typeClass;
     this._options = options;
+    this._destroyed = false;
   }
 
   get handle() {
@@ -49,6 +69,26 @@ class Entity {
   get typeClass() {
     return this._typeClass;
   }
+
+  /**
+   * Release all resources held by this entity.
+   * Do not call this method directly.
+   */
+  _destroy() {
+    if (this.isDestroyed()) return;
+
+    this._destroyed = true;
+    this.handle.release();
+    registerObsoleteHandle(this._handle);
+  }
+
+  /**
+   * Test if this entity has been destroyed and resources released.
+   * @returns {boolean} - true when destroyed has previously been called.
+   */
+  isDestroyed() {
+    return this._destroyed;
+  }
 }
 
 module.exports = Entity;

lib/node.js

@@ -38,6 +38,7 @@ const Service = require('./service.js');
 const Subscription = require('./subscription.js');
 const TimeSource = require('./time_source.js');
 const Timer = require('./timer.js');
+const Entity = require('./entity.js');
 
 // Parameter event publisher constants
 const PARAMETER_EVENT_MSG_TYPE = 'rcl_interfaces/msg/ParameterEvent';
@@ -148,26 +149,26 @@ class Node extends rclnodejs.ShadowNode {
   }
 
   execute(handles) {
-    let timersReady = this._timers.filter(
-      (timer) => handles.indexOf(timer.handle) !== -1
+    let timersReady = this._timers.filter((timer) =>
+      handles.includes(timer.handle)
     );
-    let guardsReady = this._guards.filter(
-      (guard) => handles.indexOf(guard.handle) !== -1
+    let guardsReady = this._guards.filter((guard) =>
+      handles.includes(guard.handle)
     );
-    let subscriptionsReady = this._subscriptions.filter(
-      (subscription) => handles.indexOf(subscription.handle) !== -1
+    let subscriptionsReady = this._subscriptions.filter((subscription) =>
+      handles.includes(subscription.handle)
     );
-    let clientsReady = this._clients.filter(
-      (client) => handles.indexOf(client.handle) !== -1
+    let clientsReady = this._clients.filter((client) =>
+      handles.includes(client.handle)
     );
-    let servicesReady = this._services.filter(
-      (service) => handles.indexOf(service.handle) !== -1
+    let servicesReady = this._services.filter((service) =>
+      handles.includes(service.handle)
     );
-    let actionClientsReady = this._actionClients.filter(
-      (actionClient) => handles.indexOf(actionClient.handle) !== -1
+    let actionClientsReady = this._actionClients.filter((actionClient) =>
+      handles.includes(actionClient.handle)
     );
-    let actionServersReady = this._actionServers.filter(
-      (actionServer) => handles.indexOf(actionServer.handle) !== -1
+    let actionServersReady = this._actionServers.filter((actionServer) =>
+      handles.includes(actionServer.handle)
     );
 
     timersReady.forEach((timer) => {
@@ -177,14 +178,16 @@ class Node extends rclnodejs.ShadowNode {
       }
     });
 
-    subscriptionsReady.forEach((subscription) => {
+    for (const subscription of subscriptionsReady) {
+      if (subscription.isDestroyed()) continue;
       if (subscription.isRaw) {
         let rawMessage = rclnodejs.rclTakeRaw(subscription.handle);
         if (rawMessage) {
           subscription.processResponse(rawMessage);
         }
         return;
       }
+
       this._runWithMessageType(
         subscription.typeClass,
         (message, deserialize) => {
@@ -194,13 +197,16 @@ class Node extends rclnodejs.ShadowNode {
           }
         }
       );
-    });
+    }
+
+    for (const guard of guardsReady) {
+      if (guard.isDestroyed()) continue;
 
-    guardsReady.forEach((guard) => {
       guard.callback();
-    });
+    }
 
-    clientsReady.forEach((client) => {
+    for (const client of clientsReady) {
+      if (client.isDestroyed()) continue;
       this._runWithMessageType(
         client.typeClass.Response,
         (message, deserialize) => {
@@ -213,9 +219,10 @@ class Node extends rclnodejs.ShadowNode {
           }
         }
       );
-    });
+    }
 
-    servicesReady.forEach((service) => {
+    for (const service of servicesReady) {
+      if (service.isDestroyed()) continue;
       this._runWithMessageType(
         service.typeClass.Request,
         (message, deserialize) => {
@@ -229,9 +236,11 @@ class Node extends rclnodejs.ShadowNode {
           }
         }
       );
-    });
+    }
+
+    for (const actionClient of actionClientsReady) {
+      if (actionClient.isDestroyed()) continue;
 
-    actionClientsReady.forEach((actionClient) => {
       const properties = actionClient.handle.properties;
 
       if (properties.isGoalResponseReady) {
@@ -242,7 +251,7 @@ class Node extends rclnodejs.ShadowNode {
               actionClient.handle,
               message
             );
-            if (sequence != null) {
+            if (sequence != undefined) {
               actionClient.processGoalResponse(sequence, deserialize());
             }
           }
@@ -257,7 +266,7 @@ class Node extends rclnodejs.ShadowNode {
               actionClient.handle,
               message
             );
-            if (sequence != null) {
+            if (sequence != undefined) {
               actionClient.processCancelResponse(sequence, deserialize());
             }
           }
@@ -272,7 +281,7 @@ class Node extends rclnodejs.ShadowNode {
               actionClient.handle,
               message
             );
-            if (sequence != null) {
+            if (sequence != undefined) {
               actionClient.processResultResponse(sequence, deserialize());
             }
           }
@@ -308,9 +317,11 @@ class Node extends rclnodejs.ShadowNode {
           }
         );
       }
-    });
+    }
+
+    for (const actionServer of actionServersReady) {
+      if (actionServer.isDestroyed()) continue;
 
-    actionServersReady.forEach((actionServer) => {
       const properties = actionServer.handle.properties;
 
       if (properties.isGoalRequestReady) {
@@ -371,7 +382,11 @@ class Node extends rclnodejs.ShadowNode {
         }
         GoalInfoArray.freeArray(message);
       }
-    });
+    }
+
+    // At this point it is safe to clear the cache of any
+    // destroyed entity references
+    Entity._gcHandles();
   }
 
   /**
@@ -448,11 +463,19 @@ class Node extends rclnodejs.ShadowNode {
   }
 
   _destroyEntity(entity, array, syncHandles = true) {
+    if (entity['isDestroyed'] && entity.isDestroyed()) return;
+
     this._removeEntityFromArray(entity, array);
     if (syncHandles) {
       this.syncHandles();
     }
-    entity.handle.release();
+
+    if (entity['_destroy']) {
+      entity._destroy();
+    } else {
+      // guards and timers
+      entity.handle.release();
+    }
   }
 
   _validateOptions(options) {