EventEmitter memory leak detected

[sospedra]

So, I'm running this warn: (node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.

I know that the origin is caused because in the ActionCreator I'm requesting some data and resolving async:

LOAD: function(seeds){
    request
      .post(config.routes.data)
      .type('form')
      .send({
        seeds: seeds
      })
      .end(function(err, res){
        Dispatcher.dispatch(constants.LOAD, res);
      });
    }

If I comment the Dispatcher.dispatch it works fine.

Since here all is clear, but if I shouldn't use this method I don't know what pattern should I follow to stop getting this warn, any idea?

[darcyadams]

Currently delorean creates a listener to a change event every time dispatch method is called. The change listener only gets unbound when the change event occurs. If the store does not trigger a change via either this.emit('change') or this.set('someprop', 'someval') then the listeners will build up and never be unbound. this is a known issue with delorean and fix is in the works. for now the workaround is to always respond to dispatched actions with a change event.

note that it is also possible to legitimately add more than 10 listeners, so we will be suppressing this warning for fewer than 100 listeners in the next release. I do not believe this console error actually breaks any code. It is purely meant to alert you that you may be adding listeners that are not getting cleaned up.

[sospedra]

You're absolutely right when you say that this won't break anything. But I'm curious and I like investigate. So, after reading what you say there's still one thing that I don't get.

You say that the unobserve is triggered when this.emit or this.set are launched. But, the true is that this.set is on the store:

onLoad: function(response){
    var top = response.results
      .filter(function(a){
        return a.relevance > 1;
      });

    this.set('top', top);
  }

So, I'm missing something or there's another thing here that I don't know.

P.S: Keeping in mind that this isn't 100% an issue and more about curiosity, feel free to don't answer if you are busy.

[darcyadams]

yes, when I wrote this.set and this.emit, thisis the store. So what I'm saying is that you need the method that handles the store action being dispatched to call either this.emit('change') or (if you are using delorean's scheme setup) calling this.set will automatically call this.emit('change') after the data is updated.

eg.

// In the action file you do some async action and dispatch the result to the store
LOAD: function(seeds){
  request
    .post(config.routes.data)
    .type('form')
    .send({
      seeds: seeds
    })
    .end(function(err, res){
      Dispatcher.dispatch(constants.LOAD, res);
    });
}

...

// Then in the store you have some handler for your constants.LOAD action
loadHandle: function (data) {
  //do something with data

  // emit a change event
  this.emit('change');

  // or if you are using scheme, this.set will emit the 'change' for you
  this.set('myData', data);

}

hope this helps!

[f]

I am back! @darcyadams let's start :)

[darcyadams]

Excellent! Congrats. So glad to hear it. Been slacking a bit in improving Delorean, but try to respond to every issue. I hope to dig in and get some PRs up soon.

[tommoor]

Just wanted to drop a note here, I've brought up the issue before with the event emitters too. In our application this is actually a huge deal as it stays open for days on end and if we miss a single change event across hundreds of methods in dozens of stores then that creates a memory leak - we really need to change this behaviour to fail-safe as soon as possible.

Happy to help with the work if there is an agreed direction/solution.

[darcyadams]

Agreed. Our app is running into the same issue. I have some ideas. All the issues lie within the waitFor method. I'll dig into this week and post a proposed solution.
On a high level, as long as we use store events to resolve or reject the promise returned to dispatch calls, we are going to have event cleanup issues. We could either use timers to cleanup these events (gross) or maybe track the bound events with time stamps and clean up anything older than a couple minutes when new events are bound. Ideally I can find a way to check if the event was emitted after the action handler is fired, and if it wasn't, cleanup the event then, as we would know it is no longer needed.

[sospedra]

Glad to help if you need so! Btw, sounds really bad that idea of add some timers :\ Why not use something like Promise.reject()?

[darcyadams]

Okay, PR https://github.com/deloreanjs/delorean/pull/93 is in for this issue. Appreciate all comments and feedback. I feel pretty happy with the solution. I essentially cleanup unused events after the action handler fires with a custom cleanup_{actionName} event. This solution has also allowed me to add the proper rollback functionality, which has never really been present. See PR for details.

[darcyadams]

pr is merged, published v0.9.7 to npm, closing this issue. yay!