Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Improvement to #3502 #3521

Copy link
Copy link

Description

@ryan-d-williams
Issue body actions

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!

Reactions are currently unavailable

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

    Morty Proxy This is a proxified and sanitized view of the page, visit original site.