prevent memory leaks with 'stopListening' and 'off' #3049

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
8 participants
@melnikov-s
Contributor

melnikov-s commented Mar 7, 2014

This PR fixes potential leaks when using stopListening and off, it addresses the following:

var a = _.extend({}, Backbone.Events);
var b = _.extend({}, Backbone.Events);

b.on('y', fn);
a.listenTo(b, 'x', fn);
a.stopListening(b, 'x');
b = null; // b can't be garbage collected until a.stopListening() is called
var a = _.extend({}, Backbone.Events);
var b = _.extend({}, Backbone.Events);
var c = _.extend({}, Backbone.Events);

a.listenTo(c, 'x', fn);
b.listenTo(c, 'x', fn);
c.off('x');
c = null; //c can't be garbage collected until 'stopListening()' is called on a and b

this implementation has a minimal impact on performance as it uses a counter to determine when a listener link can be safely broken.

@@ -112,42 +101,46 @@
// Remove all callbacks for all events.
if (!name && !callback && !context) {
- this._events = void 0;
+ var listenerIds = _.keys(this._listeners);
+ for (var i = 0, l = listenerIds.length; i < l; i++) {

This comment has been minimized.

Show comment Hide comment
@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

I think for (var listenerId in this._listeners) { will be more performant here.

@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

I think for (var listenerId in this._listeners) { will be more performant here.

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Mar 7, 2014

Contributor

Surprisingly, _.keys + for seems to be the fastest iteration over an object in modern browsers.
http://jsperf.com/underscore-each-vs-for-in-when-iterating-over-objects/10

it's used in the loop bellow as well.

@melnikov-s

melnikov-s Mar 7, 2014

Contributor

Surprisingly, _.keys + for seems to be the fastest iteration over an object in modern browsers.
http://jsperf.com/underscore-each-vs-for-in-when-iterating-over-objects/10

it's used in the loop bellow as well.

This comment has been minimized.

Show comment Hide comment
@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

Well so it is! And I definitely glanced over that below.

@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

Well so it is! And I definitely glanced over that below.

This comment has been minimized.

Show comment Hide comment
@stephanebachelier

stephanebachelier Mar 7, 2014

Not so surprisingly. Your test is being run against so called evergreen browsers, _.keys is just one function call to Object.keys.

For older browsers it will perform a double for loop. One for in loop for the _.keys, then the for loop on listenerIds, so it should be much slower.

But, where is the future heading ? It's still a micro benchmark. 1000 listeners to remove is a lot I think for most cases.

FYI, _.keys source code:

_.keys = function(obj) {
    if (!_.isObject(obj)) return [];
    if (nativeKeys) return nativeKeys(obj);
    var keys = [];
    for (var key in obj) if (_.has(obj, key)) keys.push(key);
    return keys;
};
@stephanebachelier

stephanebachelier Mar 7, 2014

Not so surprisingly. Your test is being run against so called evergreen browsers, _.keys is just one function call to Object.keys.

For older browsers it will perform a double for loop. One for in loop for the _.keys, then the for loop on listenerIds, so it should be much slower.

But, where is the future heading ? It's still a micro benchmark. 1000 listeners to remove is a lot I think for most cases.

FYI, _.keys source code:

_.keys = function(obj) {
    if (!_.isObject(obj)) return [];
    if (nativeKeys) return nativeKeys(obj);
    var keys = [];
    for (var key in obj) if (_.has(obj, key)) keys.push(key);
    return keys;
};

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Mar 11, 2014

Contributor

The looping logic should be rooted in what you want to allow, iterating inherited properties, or restricting to own properties of _listeners.

@jdalton

jdalton Mar 11, 2014

Contributor

The looping logic should be rooted in what you want to allow, iterating inherited properties, or restricting to own properties of _listeners.

This comment has been minimized.

Show comment Hide comment
@akre54

akre54 Jan 29, 2015

Collaborator

On a related note... Should we be avoiding _.keys in _.each? What if we branched at nativeKeys vs for ... in in each to avoid the double loop that @stephanebachelier pointed out? @jdalton

@akre54

akre54 Jan 29, 2015

Collaborator

On a related note... Should we be avoiding _.keys in _.each? What if we branched at nativeKeys vs for ... in in each to avoid the double loop that @stephanebachelier pointed out? @jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Jan 29, 2015

Contributor

The advantage to using the util lib's methods is it includes iteration fixes that you wouldn't get otherwise.

@jdalton

jdalton Jan 29, 2015

Contributor

The advantage to using the util lib's methods is it includes iteration fixes that you wouldn't get otherwise.

backbone.js
) {
remaining.push(event);
+ } else if (event.listenId && listener && --listener.count === 0){

This comment has been minimized.

Show comment Hide comment
@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

If listener is truthy, event.listenId must already be truthy right? Otherwise there is some falsey key in this._listeners which wouldn't make sense. This can probably be } else if (listener && --listener.count === 0) {

@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

If listener is truthy, event.listenId must already be truthy right? Otherwise there is some falsey key in this._listeners which wouldn't make sense. This can probably be } else if (listener && --listener.count === 0) {

backbone.js
+ this._events || (this._events = {});
+ var events = this._events[name] || (this._events[name] = []);
+ events.push({callback: callback, context: context, ctx: context || this, listenId : listenId});
+ if (listenId !== undefined) {

This comment has been minimized.

Show comment Hide comment
@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

if (listenId) { should be sufficient here.

@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

if (listenId) { should be sufficient here.

@caseywebdev

This comment has been minimized.

Show comment Hide comment
@caseywebdev

caseywebdev Mar 7, 2014

Collaborator

This has been on the radar for a while, but I think you're the first to tackle it, so thanks! I made some comments in the diff, but overall 👍. I think this bookkeeping had to be done at some point to cover the example leaks you showed.

Collaborator

caseywebdev commented Mar 7, 2014

This has been on the radar for a while, but I think you're the first to tackle it, so thanks! I made some comments in the diff, but overall 👍. I think this bookkeeping had to be done at some point to cover the example leaks you showed.

@melnikov-s

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Mar 7, 2014

Contributor

Thanks, I've amended my commit based on your comments.

Contributor

melnikov-s commented Mar 7, 2014

Thanks, I've amended my commit based on your comments.

@akre54

This comment has been minimized.

Show comment Hide comment
@akre54

akre54 Mar 9, 2014

Collaborator

Why split out _on and _once? Why not just give them both listenId as a last param?

Collaborator

akre54 commented Mar 9, 2014

Why split out _on and _once? Why not just give them both listenId as a last param?

@melnikov-s

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Mar 9, 2014

Contributor

@akre54 There's no advantage to expose listenId param to the public interface. It is very specific to the implementation, a wrong value for example could cause an exception to be thrown when off is called.

Contributor

melnikov-s commented Mar 9, 2014

@akre54 There's no advantage to expose listenId param to the public interface. It is very specific to the implementation, a wrong value for example could cause an exception to be thrown when off is called.

@akre54

This comment has been minimized.

Show comment Hide comment
@akre54

akre54 Mar 9, 2014

Collaborator

Then I would make the function internal to the closure. _on and _once are
still exposed in the public API with your change.
On Mar 9, 2014 8:19 PM, "Sergey Melnikov" notifications@github.com wrote:

@akre54 https://github.com/akre54 There's no advantage to expose
listenId param to the public interface. It is very specific to the
implementation, a wrong value for example could cause an exception to be
thrown when off is called.

Reply to this email directly or view it on GitHubhttps://github.com/jashkenas/backbone/pull/3049#issuecomment-37130775
.

Collaborator

akre54 commented Mar 9, 2014

Then I would make the function internal to the closure. _on and _once are
still exposed in the public API with your change.
On Mar 9, 2014 8:19 PM, "Sergey Melnikov" notifications@github.com wrote:

@akre54 https://github.com/akre54 There's no advantage to expose
listenId param to the public interface. It is very specific to the
implementation, a wrong value for example could cause an exception to be
thrown when off is called.

Reply to this email directly or view it on GitHubhttps://github.com/jashkenas/backbone/pull/3049#issuecomment-37130775
.

@akre54

This comment has been minimized.

Show comment Hide comment
@akre54

akre54 Mar 9, 2014

Collaborator

You might also write a documentation note to at least explain why on and
once are now very small wrappers around internals
On Mar 9, 2014 8:19 PM, "Sergey Melnikov" notifications@github.com wrote:

@akre54 https://github.com/akre54 There's no advantage to expose
listenId param to the public interface. It is very specific to the
implementation, a wrong value for example could cause an exception to be
thrown when off is called.

Reply to this email directly or view it on GitHubhttps://github.com/jashkenas/backbone/pull/3049#issuecomment-37130775
.

Collaborator

akre54 commented Mar 9, 2014

You might also write a documentation note to at least explain why on and
once are now very small wrappers around internals
On Mar 9, 2014 8:19 PM, "Sergey Melnikov" notifications@github.com wrote:

@akre54 https://github.com/akre54 There's no advantage to expose
listenId param to the public interface. It is very specific to the
implementation, a wrong value for example could cause an exception to be
thrown when off is called.

Reply to this email directly or view it on GitHubhttps://github.com/jashkenas/backbone/pull/3049#issuecomment-37130775
.

@melnikov-s

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Mar 9, 2014

Contributor

I suppose it makes sense to do that since Backbone.Events is primarily used as a mixin, don't want to create possible collisions with private methods from a base object.

What's the preferred approach to calling internal methods here? call and apply? or is it by passing in the context as the first parameter like it's done with eventsApi ?

Contributor

melnikov-s commented Mar 9, 2014

I suppose it makes sense to do that since Backbone.Events is primarily used as a mixin, don't want to create possible collisions with private methods from a base object.

What's the preferred approach to calling internal methods here? call and apply? or is it by passing in the context as the first parameter like it's done with eventsApi ?

@melnikov-s

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Mar 11, 2014

Contributor

@akre54 I've updated the PR with the suggested changes, had to go with Function.prototype.call to invoke the scoped methods or else eventsApi would have been broken.

Contributor

melnikov-s commented Mar 11, 2014

@akre54 I've updated the PR with the suggested changes, had to go with Function.prototype.call to invoke the scoped methods or else eventsApi would have been broken.

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Mar 15, 2014

Owner

Why are these being implemented as "private" internal functions?

Owner

jashkenas commented Mar 15, 2014

Why are these being implemented as "private" internal functions?

@melnikov-s

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Mar 15, 2014

Contributor

@jashkenas I've touched upon this with @akre54 but basically so that the listenerId parameter would not be exposed to the public interface.

The internal once and on were originally implemented as private methods directly on Backbone.Events but where changed to internal scope in commit 0c64d2c based on @akre54 advice as well as to prevent a possible collision in the case where Backbone.Events is used as a mixin.

Contributor

melnikov-s commented Mar 15, 2014

@jashkenas I've touched upon this with @akre54 but basically so that the listenerId parameter would not be exposed to the public interface.

The internal once and on were originally implemented as private methods directly on Backbone.Events but where changed to internal scope in commit 0c64d2c based on @akre54 advice as well as to prevent a possible collision in the case where Backbone.Events is used as a mixin.

@braddunbar

This comment has been minimized.

Show comment Hide comment
@braddunbar

braddunbar Mar 18, 2014

Collaborator

What's the use case for using stopListening to remove only certain handlers? Ostensibly, obj.stopListening(...) is used when you want to ensure obj is available for garbage collection, not objects obj listens to. If you know ahead of time that you'll want to remove handlers, why couldn't you just use off?

Collaborator

braddunbar commented Mar 18, 2014

What's the use case for using stopListening to remove only certain handlers? Ostensibly, obj.stopListening(...) is used when you want to ensure obj is available for garbage collection, not objects obj listens to. If you know ahead of time that you'll want to remove handlers, why couldn't you just use off?

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Mar 18, 2014

Owner

What's the use case for using stopListening to remove only certain handlers? Ostensibly, obj.stopListening(...) is used when you want to ensure obj is available for garbage collection, not objects obj listens to. If you know ahead of time that you'll want to remove handlers, why couldn't you just use off?

A very, very good point.

Owner

jashkenas commented Mar 18, 2014

What's the use case for using stopListening to remove only certain handlers? Ostensibly, obj.stopListening(...) is used when you want to ensure obj is available for garbage collection, not objects obj listens to. If you know ahead of time that you'll want to remove handlers, why couldn't you just use off?

A very, very good point.

@melnikov-s

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Mar 19, 2014

Contributor

@braddunbar I used stopListening with a function parameter to remove all bound events I registered on n views for my view collection object, which is what caused the memory leak. Something like:

//...
addView : function(view) {
    this.listenTo(view, 'remove', this.onRemove);
    //...
},
empty : function() {
   this.stopListening(null, 'remove', this.onRemove); 
   //I only want to unbind the events that this object was responsible for binding
   //but it causes a memory leak
   //...
}
//...
Contributor

melnikov-s commented Mar 19, 2014

@braddunbar I used stopListening with a function parameter to remove all bound events I registered on n views for my view collection object, which is what caused the memory leak. Something like:

//...
addView : function(view) {
    this.listenTo(view, 'remove', this.onRemove);
    //...
},
empty : function() {
   this.stopListening(null, 'remove', this.onRemove); 
   //I only want to unbind the events that this object was responsible for binding
   //but it causes a memory leak
   //...
}
//...
@melnikov-s

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Jan 27, 2015

Contributor

I've updated this pull request with a merge from master. I've actually ended up reverting the changes (but kept the tests) made in order to support #3226 as they were already fixed with this pull request (With no duplication in listenToOnce and eventApi required). This actually made this pull request have more line deletions than insertions within backbone.js.

Is there a reason why we're not moving forward with this PR? Any concerns that I can address ?

Contributor

melnikov-s commented Jan 27, 2015

I've updated this pull request with a merge from master. I've actually ended up reverting the changes (but kept the tests) made in order to support #3226 as they were already fixed with this pull request (With no duplication in listenToOnce and eventApi required). This actually made this pull request have more line deletions than insertions within backbone.js.

Is there a reason why we're not moving forward with this PR? Any concerns that I can address ?

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Jan 28, 2015

Owner

@braddunbar How do you feel about this now?

Owner

jashkenas commented Jan 28, 2015

@braddunbar How do you feel about this now?

backbone.js
if (
callback && callback !== event.callback &&
callback !== event.callback._callback ||
- context && context !== event.context
+ context && context !== event.context ||
+ name && name !== eventName

This comment has been minimized.

Show comment Hide comment
@jridgewell

jridgewell Jan 29, 2015

Collaborator

This name && name !== eventName condition should be taken care of by L111

@jridgewell

jridgewell Jan 29, 2015

Collaborator

This name && name !== eventName condition should be taken care of by L111

This comment has been minimized.

Show comment Hide comment
@melnikov-s

melnikov-s Jan 29, 2015

Contributor

Yup, good catch.

@melnikov-s

melnikov-s Jan 29, 2015

Contributor

Yup, good catch.

@braddunbar

This comment has been minimized.

Show comment Hide comment
@braddunbar

braddunbar Jan 30, 2015

Collaborator

I've been a bit under the weather this week, but I'll do my best to get around to this today!

Collaborator

braddunbar commented Jan 30, 2015

I've been a bit under the weather this week, but I'll do my best to get around to this today!

@jridgewell

This comment has been minimized.

Show comment Hide comment
@jridgewell

jridgewell Feb 20, 2015

Collaborator

Closing due to #3455

Collaborator

jridgewell commented Feb 20, 2015

Closing due to #3455

@jridgewell jridgewell closed this Feb 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment