I possibly found an updateNotification bug that may be responsible for the out of memory errors

[mlcampbe]

Like many have commented on here it seems that the updateNotification module will often crash and cause the MM to display a black screen and the logs to show an out of memory error. The suggestion by many has been to disable the updateNotification module. I too had this problem and this week went about looking into what was going on. I found a post that indicated the problem could be due to the module firing off update checks for ALL installed modules simultaneously. That intrigued me to think about if it was possible to use the javascript setTimeout() function to add a delay between each modules update check. In updateNotification’s node_helper.js file I added the setTimeout() around the sg.git.fetch() call with a 60sec delay added for each module to be checked:

                var i = 1;
                simpleGits.forEach(function(sg) {
                      console.log("scheduling update of " + sg.module);
                      setTimeout(function(){
                        var time = new Date();
                        console.log(time.getHours() + ":" + time.getMinutes() + ":" + time.getSeconds() + "-->" + sg.module);
                        sg.git.fetch().status(function(err, data) {
.....
                     }, 60000*i);
                     i++;
                });

This did what I expected in that the modules were updated 1 at a time with a 60sec delay between each. However, as I had the debugging statement to print out when the module was scheduled and when it actually fired I noticed something odd.

On the first updateInterval all modules were updated as expected.
On the 2nd updateInterval each modules was updated 2 times.
On the 3rd updateInterval each modules was updated 3 times.
etc …

It appears that there is a bug in the updateNotification module in that it just keeps appending all of the modules to the end of an array and on the updateInterval it checks all rows in the array. So in my case the first update fires off 6 checks at the same time, the 2nd update 12 checks, the 3rd update 18 checks, …, the 20th check 120 checks. I can see how this could easily run into an out of memory condition if there are many modules to checked and all are fired off at the same time especially on a machine such as a pi with limited memory to start with.

As a test I have taken out my setTimeout code and added in a line to clear the contents of the simpleGits array after each update. For example I now have this:

                this.scheduleNextFetch(this.config.updateInterval);
                simpleGits.length = 0;
        },

        scheduleNextFetch: function(delay) {

Has anyone seen this or think this may be the cause of the updateNotification out of memory errors?

[sdetweil]

Great catch! Certainly will lead to out of memory, and w the default cycle of 10 minutes! Wow…

A better way to clear the array is
simpleGits = []

Set to an empty array

But great find!!

[sdetweil]

hm… after more examination, simpleGits arrary should only be built once, just after DOM_OBJECTS_CREATED
then used over and over… (as u can’t install modules on the fly (yet!)).

your setTimeout schedules a routine for each module found, but the 1st is 60,000 ms, the second is 120,000ms, third is 180,000ms and so on… so they are ending sequenced… and then you create another set of timeouts again…
if there are more than 10 modules, then some of the checks will be duplicated as the ones after 10 minutes haven’t fired yet…

so, when u cleared the simpleGits array u just stopped creating more setTimeout routines…

[mlcampbe]

Yeah I think I may have jump to a conclusion too fast. In my overnight test I had the updateInterval set to 6hrs so there was no overlap of events firing as I only have 6 modules installed.

The overnight test did show it adding to the simpleGits array but subsequent testing showed that if I cleared the array as I listed then there were no subsequent updates. That would go along with the fact that the array would only be built 1 time. I am not sure what was going on initially.

I’ve removed all my added code except the console.log message to see when the updates are called and what modules are updated. I’ll monitor it for a bit and see what happens.

[mlcampbe]

@mlcampbe
I checked my logs this morning and with the updateInterval set to 1hr I see some fishy updates.

19:05 - checks all 7 modules 2 times
20:05 - checks all 7 modules 3 times
21:05 - checks all 7 modules 3 times
21:32 - checks all 7 modules 3 times
22:32 - checks all 7 modules 8 times
0:32 - checks all 7 modules 4 times
1:32 - checks all 7 modules 4 times

So there is something odd going on. After I restarted MM via the pm2 process I then access MM via browser on a local machine and it refreshes all modules and I can see that the number of times updateNotification checks the modules gets incremented by 1. Every time I refresh the browser it prints out my modules and clearly this scenario is causing the simpleGits array to be appended to and all modules to be checked n+1 times.

As a fix I added code to save the incoming module list (passed into socketNotificationReceived) like this:

        socketNotificationReceived: function (notification, payload) {
                if (notification === "CONFIG") {
                        this.config = payload;
                        savedPayload = payload;
                } else if(notification === "MODULES") {
                        this.configureModules(payload);
                        this.preformFetch();
                }
        },

Now the variable savedPayload is re-used at the bottom of the preformFetch:

                        sg.git.fetch().status(function(err, data) {
                                data.module = sg.module;
                                if (!err) {
                                        sg.git.log({"-1": null}, function(err, data2) {
                                                data.hash = data2.latest.hash;
                                                self.sendSocketNotification("STATUS", data);
                                        });
                                }
                        });
                });
                simpleGits = [];
                this.configureModules(savedPayload);
                this.scheduleNextFetch(this.config.updateInterval);
        },

That seems to work for browser refreshes as I can see that no matter how many times I refresh it there is only a single check of each module. I’ll monitor updateInterval refreshes to see what occurs.

[sdetweil]

@mlcampbe yes, MM screen refresh causes LOTS of problems… most modules cannot handle this…
(the MM design is that it should never be done)

access remote OR local but not both…

[sdetweil]

@mlcampbe i don’t think u needed to save the payload… i think clearing the array should be enough…

the payload should be the same, as the module.js is just getting a list of installed/running modules.

actually a more simple way, is check to see if the timer is not started,. then allow config, else skip it.
then it will only be done once…

basically change this

	socketNotificationReceived: function (notification, payload) {
		if (notification === "CONFIG") {
			this.config = payload;
		} else if(notification === "MODULES") {
			this.configureModules(payload);
			this.preformFetch();
		}
	},

to this

	socketNotificationReceived: function (notification, payload) {
		if (notification === "CONFIG") {
			this.config = payload;
		} else if(notification === "MODULES") {
                      // if refresh cycle timer not yet started
                      if(this.updateTimer==null ){
			this.configureModules(payload);
			this.preformFetch();
                      }
		}
	},

this still leaves a tiny window, if the initial cycle is running, and you start another via refresh…
so, this calls for another flag.
updateProcessStarted: false
and this

	socketNotificationReceived: function (notification, payload) {
		if (notification === "CONFIG") {
			this.config = payload;
		} else if(notification === "MODULES") {
                      // if this is the 1st time thru the update check process
                      if(this.updateProcessStarted==false ){
                        this.updateProcessStarted=true;
			this.configureModules(payload);
			this.preformFetch();
                      }
		}
	},

[mlcampbe]

Sam, when I cleared the simpleGits array only then nothing was refreshing. That could have been where I was clearing it though. It seems that as you mentioned the configureModules was only getting called 1 time when initially loaded unless of course I accessed MM via the browser which called everything again.

For now I am happy with my solution as it is working for both auto refreshes via the updateInterval and for browser access from another machine. Its not any more code than you added but just done a different way. It may not be quite as efficient as it builds the simpleGits array everytime it is called but I don’t plan to call it more than once every 6 hrs anyway so that is not going to be a problem.

It is yet to be seen if this fixes the out of memory errors that were being seen though. I’ll have to leave it running for a day or 2 and check to see what happens.

[sdetweil]

ok, I have mine running too… shows 1 update available…

[mlcampbe]

This post is deleted!

[sdetweil]

I’ve spent the day looking over the module, and there are a number of bugs that had to be fixed.

the design is synchronous calls to simple-gits module, but simple-gits uses promises under the covers, so everything is actually async, which exposes some potential data corruption

also the module.js overwrites the this.status pointer on each notification from the helper, before checking if the data is useful (behind > 0)… which yanks the data out from under getDom().

and getDom() processes for each good data message, overlaying anything prior… only one module status shows

i am testing fixes for all of that…

[ezarlive]

Make a pull request ;)

[sdetweil]

@ezarlive testing… before sending the code