From f7210effec875c1db426003019f546999b151716 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Mon, 26 Apr 2021 21:14:42 +0100 Subject: [PATCH] Rework hooks structure to be a linkedlist Allows for safe removal of hooks whilst they are being invoked --- .../node_modules/@node-red/util/lib/hooks.js | 87 +++++++++++++------ test/unit/@node-red/util/lib/hooks_spec.js | 39 +++++++++ 2 files changed, 100 insertions(+), 26 deletions(-) diff --git a/packages/node_modules/@node-red/util/lib/hooks.js b/packages/node_modules/@node-red/util/lib/hooks.js index a49104813..59a8749c4 100644 --- a/packages/node_modules/@node-red/util/lib/hooks.js +++ b/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 (labelledHooks[label] && labelledHooks[label][id]) { - throw new Error("Hook "+hookId+" already registered") - } - labelledHooks[label] = labelledHooks[label]||{}; - labelledHooks[label][id] = callback; + if (label && labelledHooks[label] && labelledHooks[label][id]) { + throw new Error("Hook "+hookId+" already registered") } // 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] || []; - hooks[id].push({cb:callback,location:callModule}); + const hookItem = {cb:callback, location: callModule, previousHook: null, nextHook: null } + + 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) { - let i = hooks[id].findIndex(hook => hook.cb === callback); - if (i !== -1) { - hooks[id].splice(i,1); - if (hooks[id].length === 0) { - delete hooks[id]; - delete states[id]; - } +function removeHook(id,hookItem) { + let previousHook = hookItem.previousHook; + let nextHook = hookItem.nextHook; + + 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]; - if (!hookStack || hookStack.length === 0) { + let hookItem = hooks[hookId]; + 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) { - let i = 0; +function invokeStack(hookItem,payload,done) { function callNextHook(err) { - if (i === hookStack.length || err) { + if (!hookItem || err) { done(err); return; } - const hook = hookStack[i++]; - const callback = hook.cb; + if (hookItem.removed) { + 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); diff --git a/test/unit/@node-red/util/lib/hooks_spec.js b/test/unit/@node-red/util/lib/hooks_spec.js index 121486ade..4b25f33d2 100644 --- a/test/unit/@node-red/util/lib/hooks_spec.js +++ b/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 } )