From b828a3487b4d25c108462e77dd3366599a232b1c Mon Sep 17 00:00:00 2001 From: Joe Pavitt Date: Wed, 18 Oct 2023 11:55:46 +0100 Subject: [PATCH 1/2] Fix socket connection cleanup & re-establish in constructor --- nodes/config/ui_base.js | 116 ++++++++++++++++++++++--------------- nodes/config/ui_group.js | 2 +- nodes/config/ui_page.js | 2 +- nodes/config/ui_theme.js | 3 +- nodes/widgets/ui_button.js | 3 +- 5 files changed, 73 insertions(+), 53 deletions(-) diff --git a/nodes/config/ui_base.js b/nodes/config/ui_base.js index db18abc9e..aa5ca05e7 100644 --- a/nodes/config/ui_base.js +++ b/nodes/config/ui_base.js @@ -183,15 +183,21 @@ module.exports = function (RED) { * @param {Object} n - Node-RED node configuration as entered in the nodes editor */ function UIBaseNode (n) { + RED.nodes.createNode(this, n) const node = this - RED.nodes.createNode(node, n) + + node._created = Date.now() /** @type {Object.} */ - node.connections = {} // store socket.io connections for this node - // re-map existing connections for this base node + // node.connections = {} // store socket.io connections for this node + // // re-map existing connections for this base node for (const id in uiShared.connections) { + const socket = uiShared.connections[id] if (uiShared.connections[id]._baseId === node.id) { - node.connections[id] = uiShared.connections[id] + // re establish event handlers + socket.on('widget-action', onAction.bind(null, socket)) + socket.on('widget-change', onChange.bind(null, socket)) + socket.on('widget-load', onLoad.bind(null, socket)) } } /** @type {NodeJS.Timeout} */ @@ -216,43 +222,35 @@ module.exports = function (RED) { }) } - /** - * on connection handler for SocketIO - * @param {Socket} socket socket.io socket connecting to the server - */ - function onConnection (socket) { - // record mapping from connection to he ui-base node - socket._baseId = node.id - - node.connections[socket.id] = socket // store the connection for later use - uiShared.connections[socket.id] = socket // store the connection for later use - emitConfig(socket) + // remove event handler socket listeners for a given socket connection + function cleanupEventHandlers (socket) { + try { + socket.removeAllListeners('widget-action') + } catch (_error) { /* do nothing */ } + try { + socket.removeAllListeners('widget-change') + } catch (_error) { /* do nothing */ } + try { + socket.removeAllListeners('widget-load') + } catch (_error) { /* do nothing */ } + try { + socket.removeAllListeners('disconnect') + } catch (_error) { /* do nothing */ } - const cleanup = () => { - try { - socket.removeListener('widget-action', onAction.bind(null, socket)) - } catch (_error) { /* do nothing */ } - try { - socket.removeListener('widget-change', onChange.bind(null, socket)) - } catch (_error) { /* do nothing */ } - try { - socket.removeListener('widget-load', onLoad.bind(null, socket)) - } catch (_error) { /* do nothing */ } - - // check if any widgets have defined custom socket events - // remove their listeners to make sure we clean up properly - node.ui?.widgets?.forEach((widget) => { - if (widget.hooks?.onSocket) { - for (const [eventName, handler] of Object.entries(widget.hooks.onSocket)) { - try { - socket.removeListener(eventName, handler) - } catch (_error) { /* do nothing */ } - } + // check if any widgets have defined custom socket events + // remove their listeners to make sure we clean up properly + node.ui?.widgets?.forEach((widget) => { + if (widget.hooks?.onSocket) { + for (const [eventName] of Object.entries(widget.hooks.onSocket)) { + try { + socket.removeAllListeners(eventName) + } catch (_error) { /* do nothing */ } } - }) - } - // clean up then re-register listeners - cleanup() + } + }) + } + + function setupEventHandlers (socket) { socket.on('widget-action', onAction.bind(null, socket)) socket.on('widget-change', onChange.bind(null, socket)) socket.on('widget-load', onLoad.bind(null, socket)) @@ -269,12 +267,28 @@ module.exports = function (RED) { // handle disconnection socket.on('disconnect', reason => { - cleanup() + cleanupEventHandlers(socket) delete uiShared.connections[socket.id] - delete node.connections[socket.id] node.log(`Disconnected ${socket.id} due to ${reason}`) }) } + + /** + * on connection handler for SocketIO + * @param {Socket} socket socket.io socket connecting to the server + */ + function onConnection (socket) { + // record mapping from connection to he ui-base node + socket._baseId = node.id + + // node.connections[socket.id] = socket // store the connection for later use + uiShared.connections[socket.id] = socket // store the connection for later use + emitConfig(socket) + + // clean up then re-register listeners + cleanupEventHandlers(socket) + setupEventHandlers(socket) + } /** * Handles a widget-action event from the UI * @param {Socket} conn - socket.io socket connecting to the server @@ -390,6 +404,8 @@ module.exports = function (RED) { * @returns {Object} - { wNode, widgetConfig, widgetEvents, widget } */ function getWidgetAndConfig (id) { + // node.ui?.widgets is empty? + // themes, groups, etc. are not empty? const wNode = RED.nodes.getNode(id) const widget = node.ui?.widgets?.get(id) const widgetConfig = widget?.props || {} @@ -403,6 +419,10 @@ module.exports = function (RED) { // Make sure we clean up after ourselves node.on('close', (removed, done) => { uiShared.ioServer?.off('connection', onConnection) + for (const [id, conn] of Object.entries(uiShared.connections)) { + cleanupEventHandlers(conn) + // delete node.connections[conn.id] + } close(node, function (err) { if (err) { node.error(`Error closing socket.io server for ${node.id}`, err) @@ -438,7 +458,7 @@ module.exports = function (RED) { node.emitConfigRequested = setTimeout(() => { try { // emit config to all connected UI for this ui-base - Object.values(node.connections).forEach(socket => { + Object.values(uiShared.connections).forEach(socket => { emitConfig(socket) }) } finally { @@ -546,11 +566,6 @@ module.exports = function (RED) { * Event Handlers */ - widgetNode.on('close', function (removed, done) { - node.deregister(null, null, widgetNode) - done() - }) - // add Node-RED listener to the widget for when it's corresponding node receives a msg in Node-RED widgetNode.on('input', async function (msg, send, done) { // ensure we have latest instance of the widget's node @@ -600,6 +615,13 @@ module.exports = function (RED) { } } }) + + // when a widget is "closed" remove it from this Base Node's knowledge + widgetNode.on('close', function (removed, done) { + node.deregister(null, null, widgetNode) + done() + }) + node.requestEmitConfig() // queue up a config emit to the UI } diff --git a/nodes/config/ui_group.js b/nodes/config/ui_group.js index 30b14f2a4..79a2f0607 100644 --- a/nodes/config/ui_group.js +++ b/nodes/config/ui_group.js @@ -4,8 +4,8 @@ module.exports = function (RED) { * @param {*} config */ function UIGroupNode (config) { + RED.nodes.createNode(this, config) const node = this - RED.nodes.createNode(node, config) node.on('close', function (removed, done) { node.deregister() // deregister self diff --git a/nodes/config/ui_page.js b/nodes/config/ui_page.js index a9aa7aaf7..e8dc63d99 100644 --- a/nodes/config/ui_page.js +++ b/nodes/config/ui_page.js @@ -4,8 +4,8 @@ module.exports = function (RED) { * @param {*} config */ function UIPageNode (config) { + RED.nodes.createNode(this, config) const node = this - RED.nodes.createNode(node, config) node.on('close', function (removed, done) { node.deregister() // deregister self diff --git a/nodes/config/ui_theme.js b/nodes/config/ui_theme.js index cea9659bf..aa4711740 100644 --- a/nodes/config/ui_theme.js +++ b/nodes/config/ui_theme.js @@ -4,13 +4,12 @@ module.exports = function (RED) { * @param {*} config */ function UIThemeNode (config) { + RED.nodes.createNode(this, config) const node = this // eslint-disable-next-line no-unused-vars const { id, name, type, _users, ...rest } = config node.colors = { ...rest.colors } - - RED.nodes.createNode(node, config) } RED.nodes.registerType('ui-theme', UIThemeNode) } diff --git a/nodes/widgets/ui_button.js b/nodes/widgets/ui_button.js index 67acafbb5..5baa6b89b 100644 --- a/nodes/widgets/ui_button.js +++ b/nodes/widgets/ui_button.js @@ -1,9 +1,8 @@ module.exports = function (RED) { function ButtonNode (config) { - const node = this - // create node in Node-RED RED.nodes.createNode(this, config) + const node = this // which group are we rendering this widget const group = RED.nodes.getNode(config.group) From e1f0e146071236e468637516e5290a5d89faa27c Mon Sep 17 00:00:00 2001 From: Joe Pavitt Date: Wed, 18 Oct 2023 13:02:14 +0100 Subject: [PATCH 2/2] linting: unused var --- nodes/config/ui_base.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/nodes/config/ui_base.js b/nodes/config/ui_base.js index aa5ca05e7..1a7fc1353 100644 --- a/nodes/config/ui_base.js +++ b/nodes/config/ui_base.js @@ -419,9 +419,8 @@ module.exports = function (RED) { // Make sure we clean up after ourselves node.on('close', (removed, done) => { uiShared.ioServer?.off('connection', onConnection) - for (const [id, conn] of Object.entries(uiShared.connections)) { + for (const conn of Object.values(uiShared.connections)) { cleanupEventHandlers(conn) - // delete node.connections[conn.id] } close(node, function (err) { if (err) {