Memory Leak in the action method

[thoro]

https://github.com/pipobscure/NodeJS-AsteriskManager/commit/78371238e261d6159985abe2a15cc0cba9afffd5

I would say this change introduces a memory leak, since the events for the action is never cleared.

[igorescobar]

Hi @Thoro could you share a piece of code that is exemplifying the "memory leak" problem?

[thoro]

here's an example code to simulate the once vs on assignment, if the user does not explicitly unregister the handler it will increase the memory usage over time.

var evt = require('events');

var callback = function (v) {
    console.log(process.memoryUsage(), v);
}

var emitter = new evt.EventEmitter();

var totalCount = 1000000;

var simulateTick = function () {

    emitter.emit('action_id_' + totalCount, totalCount);

    if (totalCount == 0) {
        return;
    }

    totalCount--;

   // Change here to once
    emitter.on('action_id_' + totalCount, callback);

    setImmediate(simulateTick);
}

setImmediate(simulateTick);

with on { rss: 228790272, heapTotal: 217760312, heapUsed: 180481392 } 6 { rss: 228790272, heapTotal: 217760312, heapUsed: 180488864 } 5 { rss: 228790272, heapTotal: 217760312, heapUsed: 180496336 } 4 { rss: 228790272, heapTotal: 217760312, heapUsed: 180503808 } 3 { rss: 228790272, heapTotal: 217760312, heapUsed: 180511280 } 2 { rss: 228790272, heapTotal: 217760312, heapUsed: 180518752 } 1 { rss: 228790272, heapTotal: 217760312, heapUsed: 180526224 } 0

with once { rss: 54108160, heapTotal: 42780928, heapUsed: 9964824 } 6 { rss: 54108160, heapTotal: 42780928, heapUsed: 9972624 } 5 { rss: 54108160, heapTotal: 42780928, heapUsed: 9980424 } 4 { rss: 54108160, heapTotal: 42780928, heapUsed: 9988224 } 3 { rss: 54108160, heapTotal: 42780928, heapUsed: 9996024 } 2 { rss: 54108160, heapTotal: 42780928, heapUsed: 10003824 } 1 { rss: 54108160, heapTotal: 42780928, heapUsed: 10011632 } 0

[igorescobar]

Thank you @Thoro . @2naive I think we need to change our implementation @Thoro is right...

[igorescobar]

@Thoro

What about if we do this:

  this.on(id, callback);

  setTimeout(function(_, id, callback){
    _.removeListener(id, callback);  
  }, 5000, this, id, callback)

  return context.lastid = id;

In that way... we will keep the action chain and still "cleanup" unused listeners.

[igorescobar]

The setTimeout interval can be increased in like, 30 seconds or more in my opinion.

[2naive]

http://nodejs.org/api/events.html#events_emitter_on_event_listener on Adds a listener to the end of the listeners array for the specified event. No checks are made to see if the listener has already been added. Multiple calls passing the same combination of event and listener will result in the listener being added multiple times.

once Adds a one time listener for the event. This listener is invoked only the next time the event is fired, after which it is removed.

In the example above, as I understand, in case of on it has added listener 1000000 times but never removed.

In case of once it was removed after was fired.

Could you explain what's the problem in original code? We add on listener just 1 time, and?

@igorescobar I think timeout isn't the best decision.

[igorescobar]

@2naive the problem is that we attach a listener once for each generated action id and it's never removed .... it will be forever listening for an event that will never occur again... so memory usage will increase forever...

I agree that timeout isn't the best decision but I can't see how to solve it right now without breaking things.

[igorescobar]

Nowdays we're receiving 600 calls (simultaneously)... We can easily achieve the mark of 1000000 listeners without being used.

[igorescobar]

@Thoro can you test our latest version of ami.js? https://github.com/pipobscure/NodeJS-AsteriskManager/commit/980de759e55d9a1c8657233d901e42f5cc7bf7b2

[thoro]

@igorescobar Nope :) - don't have it running just checked the commits because I was interested and saw it there.

[igorescobar]

@Thoro LOL @2naive could you?

[2naive]

@igorescobar why shouldn`t we just let users to remove listener when it's no longer needed and write it to docs?

I may do, but just only on weekend...

[pipobscure]

This seems fixed in 0.1.15