I discovered the issue described in #3502 this morning and applied a patch locally. While creating an issue with the fix, I noticed it had already been solved. Looking over the commit, it looks like that fix was applied pretty far downstream (by downstream I mean the function stack is start -> loadModules -> loadModule) and inside loadModule we are performing an unnecessary file system operation. It seems as though we can prevent this loading of the same module well upstream in the start function with a one-line change (this is from lines ~260 in app.js with the only change being the new if statement - the rest of the code is copied for readability):
for (const module of config.modules) {
if (module.disabled) continue;
if (module.module) {
if (Utils.moduleHasValidPosition(module.position) || typeof (module.position) === "undefined") {
if(!modules.includes(module.module)){ // ONLY NEW ADDITION
modules.push(module.module);
}
} else {
Log.warn("Invalid module position found for this configuration:", module);
}
} else {
Log.warn("No module name found for this configuration:", module);
}
}
This fix should prevent any unnecessary filesystem access and prevent extraneous code from being run on startup.
Additionally, while tracking down this bug, I found the following code in the base node_helper.js around line 85:
io.of(this.name).on("connection", (socket) => {
// add a catch all event.
const onevent = socket.onevent;
socket.onevent = function (packet) {
const args = packet.data || [];
onevent.call(this, packet); // original call
packet.data = ["*"].concat(args);
onevent.call(this, packet); // additional call to catch-all
};
// register catch all.
socket.on("*", (notification, payload) => {
if (notification !== "*") {
this.socketNotificationReceived(notification, payload);
}
});
});
This is a fairly old method of catch-all event handling in socketIO. It may have been borrowed from a 2015 SO answer. New versions of socketIO make this much simpler [docs]:
io.of(this.name).on("connection", (socket) => {
// register catch all.
socket.onAny((notification, payload) => {
this.socketNotificationReceived(notification, payload);
});
});
I'm happy to submit pull requests for both of these topics, but I wanted to make sure it's discussed here since they are both critical areas of the setup.
Please let me know if I am missing anything!
I discovered the issue described in #3502 this morning and applied a patch locally. While creating an issue with the fix, I noticed it had already been solved. Looking over the commit, it looks like that fix was applied pretty far downstream (by downstream I mean the function stack is
start->loadModules->loadModule) and insideloadModulewe are performing an unnecessary file system operation. It seems as though we can prevent this loading of the same module well upstream in thestartfunction with a one-line change (this is from lines ~260 inapp.jswith the only change being the new if statement - the rest of the code is copied for readability):This fix should prevent any unnecessary filesystem access and prevent extraneous code from being run on startup.
Additionally, while tracking down this bug, I found the following code in the base
node_helper.jsaround line 85:This is a fairly old method of catch-all event handling in socketIO. It may have been borrowed from a 2015 SO answer. New versions of socketIO make this much simpler [docs]:
I'm happy to submit pull requests for both of these topics, but I wanted to make sure it's discussed here since they are both critical areas of the setup.
Please let me know if I am missing anything!