Rework hooks structure to be a linkedlist

Allows for safe removal of hooks whilst they are being invoked

packages/node_modules/@node-red/util/lib/hooks.js

@@ -20,7 +20,16 @@ const VALID_HOOKS = [
 // Flags for what hooks have handlers registered
 let states = { }
 
-// Hooks by id
+// Doubly-LinkedList of hooks by id.
+// - hooks[id] points to head of list
+// - each list item looks like:
+//   {
+//       cb: the callback function
+//       location: filename/line of code that added the hook
+//       previousHook: reference to previous hook in list
+//       nextHook: reference to next hook in list
+//       removed: a flag that is set if the item was removed
+//   }
 let hooks = { }
 
 // Hooks by label
@@ -54,20 +63,33 @@ function add(hookId, callback) {
     if (VALID_HOOKS.indexOf(id) === -1) {
         throw new Error(`Invalid hook '${id}'`);
     }
-    if (label) {
+    if (label && labelledHooks[label] && labelledHooks[label][id]) {
-        if (labelledHooks[label] && labelledHooks[label][id]) {
         throw new Error("Hook "+hookId+" already registered")
     }
-        labelledHooks[label] = labelledHooks[label]||{};
-        labelledHooks[label][id] = callback;
-    }
     // Get location of calling code
     const stack = new Error().stack;
     const callModule = stack.split("\n")[2].split("(")[1].slice(0,-1);
     Log.debug(`Adding hook '${hookId}' from ${callModule}`);
 
-    hooks[id] = hooks[id] || [];
+    const hookItem = {cb:callback, location: callModule, previousHook: null, nextHook: null }
-    hooks[id].push({cb:callback,location:callModule});
+
+    let tailItem = hooks[id];
+    if (tailItem === undefined) {
+        hooks[id] = hookItem;
+    } else {
+        while(tailItem.nextHook !== null) {
+            tailItem = tailItem.nextHook
+        }
+        tailItem.nextHook = hookItem;
+        hookItem.previousHook = tailItem;
+    }
+
+    if (label) {
+        labelledHooks[label] = labelledHooks[label]||{};
+        labelledHooks[label][id] = hookItem;
+    }
+
+    // TODO: get rid of this;
     states[id] = true;
 }
 
@@ -100,21 +122,29 @@ function remove(hookId) {
     }
 }
 
-function removeHook(id,callback) {
+function removeHook(id,hookItem) {
-    let i = hooks[id].findIndex(hook => hook.cb === callback);
+    let previousHook = hookItem.previousHook;
-    if (i !== -1) {
+    let nextHook = hookItem.nextHook;
-        hooks[id].splice(i,1);
+
-        if (hooks[id].length === 0) {
+    if (previousHook) {
+        previousHook.nextHook = nextHook;
+    } else {
+        hooks[id] = nextHook;
+    }
+    if (nextHook) {
+        nextHook.previousHook = previousHook;
+    }
+    hookItem.removed = true;
+    if (!previousHook && !nextHook) {
         delete hooks[id];
         delete states[id];
     }
-    }
 }
 
 
 function trigger(hookId, payload, done) {
-    const hookStack = hooks[hookId];
+    let hookItem = hooks[hookId];
-    if (!hookStack || hookStack.length === 0) {
+    if (!hookItem) {
         if (done) {
             done();
             return;
@@ -124,7 +154,7 @@ function trigger(hookId, payload, done) {
     }
     if (!done) {
         return new Promise((resolve,reject) => {
-            invokeStack(hookStack,payload,function(err) {
+            invokeStack(hookItem,payload,function(err) {
                 if (err !== undefined && err !== false) {
                     if (!(err instanceof Error)) {
                         err = new Error(err);
@@ -137,18 +167,21 @@ function trigger(hookId, payload, done) {
             })
         });
     } else {
-        invokeStack(hookStack,payload,done)
+        invokeStack(hookItem,payload,done)
     }
 }
-function invokeStack(hookStack,payload,done) {
+function invokeStack(hookItem,payload,done) {
-    let i = 0;
     function callNextHook(err) {
-        if (i === hookStack.length || err) {
+        if (!hookItem || err) {
             done(err);
             return;
         }
-        const hook = hookStack[i++];
+        if (hookItem.removed) {
-        const callback = hook.cb;
+            hookItem = hookItem.nextHook;
+            callNextHook();
+            return;
+        }
+        const callback = hookItem.cb;
         if (callback.length === 1) {
             try {
                 let result = callback(payload);
@@ -161,6 +194,7 @@ function invokeStack(hookStack,payload,done) {
                     result.then(handleResolve, callNextHook)
                     return;
                 }
+                hookItem = hookItem.nextHook;
                 callNextHook();
             } catch(err) {
                 done(err);
@@ -177,6 +211,7 @@ function invokeStack(hookStack,payload,done) {
     }
     function handleResolve(result) {
         if (result === undefined) {
+            hookItem = hookItem.nextHook;
             callNextHook();
         } else {
             done(result);

test/unit/@node-red/util/lib/hooks_spec.js

@@ -121,7 +121,46 @@ describe("util/hooks", function() {
             })
         })
     })
+    it("allows a hook to remove itself whilst being called", function(done) {
+        let data = { order: [] }
+        hooks.add("onSend.A", function(payload) { payload.order.push("A") } )
+        hooks.add("onSend.B", function(payload) {
+            hooks.remove("*.B");
+        })
+        hooks.add("onSend.C", function(payload) { payload.order.push("C") } )
+        hooks.add("onSend.D", function(payload) { payload.order.push("D") } )
 
+        hooks.trigger("onSend", data, err => {
+            try {
+                should.not.exist(err);
+                data.order.should.eql(["A","C","D"])
+                done();
+            } catch(e) {
+                done(e);
+            }
+        })
+    });
+
+    it("allows a hook to remove itself and others whilst being called", function(done) {
+        let data = { order: [] }
+        hooks.add("onSend.A", function(payload) { payload.order.push("A") } )
+        hooks.add("onSend.B", function(payload) {
+            hooks.remove("*.B");
+            hooks.remove("*.C");
+        })
+        hooks.add("onSend.C", function(payload) { payload.order.push("C") } )
+        hooks.add("onSend.D", function(payload) { payload.order.push("D") } )
+
+        hooks.trigger("onSend", data, err => {
+            try {
+                should.not.exist(err);
+                data.order.should.eql(["A","D"])
+                done();
+            } catch(e) {
+                done(e);
+            }
+        })
+    });
 
     it("halts execution on return false", function(done) {
         hooks.add("onSend.A", function(payload) { payload.order.push("A"); return false } )