Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix listenTo memory leak #3455

Merged
merged 16 commits into from
Feb 3, 2015
Merged
230 changes: 126 additions & 104 deletions backbone.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,74 +81,24 @@
// Bind an event to a `callback` function. Passing `"all"` will bind
// the callback to all events fired.
on: function(name, callback, context) {
if (!eventsApi(this, 'on', name, [callback, context]) || !callback) return this;
this._events || (this._events = {});
var events = this._events[name] || (this._events[name] = []);
events.push({callback: callback, context: context, ctx: context || this});
this._events = eventsApi(onApi, this._events || {}, name, callback, context, this);
return this;
},

// Bind an event to only be triggered a single time. After the first time
// the callback is invoked, it will be removed.
once: function(name, callback, context) {
if (!eventsApi(this, 'once', name, [callback, context]) || !callback) return this;
var self = this;
var once = _.once(function() {
self.off(name, once);
callback.apply(this, arguments);
});
once._callback = callback;
return this.on(name, once, context);
name = onceMap(name, callback, _.bind(this.off, this));
return this.on(name, callback, context);
},

// Remove one or many callbacks. If `context` is null, removes all
// callbacks with that function. If `callback` is null, removes all
// callbacks for the event. If `name` is null, removes all bound
// callbacks for all events.
off: function(name, callback, context) {
if (!this._events || !eventsApi(this, 'off', name, [callback, context])) return this;

// Remove all callbacks for all events.
if (!name && !callback && !context) {
this._events = void 0;
return this;
}

var names = name ? [name] : _.keys(this._events);
for (var i = 0, length = names.length; i < length; i++) {
name = names[i];

// Bail out if there are no events stored.
var events = this._events[name];
if (!events) continue;

// Remove all callbacks for this event.
if (!callback && !context) {
delete this._events[name];
continue;
}

// Find any remaining events.
var remaining = [];
for (var j = 0, k = events.length; j < k; j++) {
var event = events[j];
if (
callback && callback !== event.callback &&
callback !== event.callback._callback ||
context && context !== event.context
) {
remaining.push(event);
}
}

// Replace events if there are any remaining. Otherwise, clean up.
if (remaining.length) {
this._events[name] = remaining;
} else {
delete this._events[name];
}
}

if (!this._events) return this;
this._events = eventsApi(offApi, this._events, name, callback, context);
return this;
},

Expand All @@ -159,60 +109,55 @@
trigger: function(name) {
if (!this._events) return this;
var args = slice.call(arguments, 1);
if (!eventsApi(this, 'trigger', name, args)) return this;
var events = this._events[name];
var allEvents = this._events.all;
if (events) triggerEvents(events, args);
if (allEvents) triggerEvents(allEvents, arguments);

// Use `eventsApi` to normalize `name` into the proper event names.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind clarifying this? I'm not sure what this is getting at.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jridgewell the comments are still lacking. Can you do a pass to make sure the logic is clear? I'm finding it hard to follow what is happening and why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't revised the comments yet, was trying to solve the off problem first.

Is there a particular spot you're not able to follow?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line in particular isn't clear what it does, but the bits with triggerSentinel and the numerous internal functions mostly.

It almost seems like we should drop the Events = {on: ..., off: ..., listenTo: ...} object, and just colocate the different events methods (Events.on = ...; onApi = ...; Events.off = ...;.) To follow this logic, I'm having to jump back and forth through the source to even understand where the different pieces are going.

There's so much extra overhead and indirection going on here, it really detracts from the readability of the Events code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the source could be better arranged, I'll work on that tonight along with the comments.

There's so much extra overhead and indirection going on here

I actually thought it was kind of easier to follow without the old eventsApi behavior, but then I had to account for off cleaning up any listeners. It makes it easier in my mind to think of all the *Api functions as special purpose reduce iteratees, with eventsApi being _.reduce.

eventsApi(triggerApi, this, name, triggerSentinel, args);
return this;
},

// Inversion-of-control versions of `on` and `once`. Tell *this* object to
// listen to an event in another object ... keeping track of what it's
// listening to.
listenTo: function(obj, name, callback) {
var listeningTo = this._listeningTo || (this._listeningTo = {});
if (!obj) return this;
var id = obj._listenId || (obj._listenId = _.uniqueId('l'));
listeningTo[id] = obj;
if (!callback && typeof name === 'object') callback = this;
var listeningTo = this._listeningTo || (this._listeningTo = {});
var listenee = listeningTo[id] || (listeningTo[id] = {obj: obj, events: {}});

// Bind callbacks on obj, and keep track of them on listenee.
obj.on(name, callback, this);
listenee.events = eventsApi(onApi, listenee.events, name, callback);
return this;
},

listenToOnce: function(obj, name, callback) {
if (typeof name === 'object') {
for (var event in name) this.listenToOnce(obj, event, name[event]);
return this;
}
if (eventSplitter.test(name)) {
var names = name.split(eventSplitter);
for (var i = 0, length = names.length; i < length; i++) {
this.listenToOnce(obj, names[i], callback);
}
return this;
}
if (!callback) return this;
var once = _.once(function() {
this.stopListening(obj, name, once);
callback.apply(this, arguments);
});
once._callback = callback;
return this.listenTo(obj, name, once);
name = onceMap(name, callback, _.bind(this.stopListening, this, obj));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it not also be bound to name, callback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't, because name could be an object or space-separated string. This will be passed in the normalized (just a single event name and its callback) by the onceWrapper.

return this.listenTo(obj, name, callback);
},

// Tell this object to stop listening to either specific events ... or
// to every object it's currently listening to.
stopListening: function(obj, name, callback) {
var listeningTo = this._listeningTo;
if (!listeningTo) return this;
var remove = !name && !callback;
if (!callback && typeof name === 'object') callback = this;
if (obj) (listeningTo = {})[obj._listenId] = obj;
for (var id in listeningTo) {
obj = listeningTo[id];
obj.off(name, callback, this);
if (remove || _.isEmpty(obj._events)) delete this._listeningTo[id];

var listeneeIds = obj ? [obj._listenId] : _.keys(listeningTo);
for (var i = 0, length = listeneeIds.length; i < length; i++) {
var id = listeneeIds[i];
var listenee = listeningTo[id];

// If listenee doesn't exist, this object is not currently
// listening to obj. Break out early.
if (!listenee) break;

listenee.obj.off(name, callback, this);

// Events will only ever be falsey if all the event callbacks
// are removed. If so, stop delete the listenee.
var events = eventsApi(offApi, listenee.events, name, callback);
if (!events) delete listeningTo[id];
}
if (_.isEmpty(listeningTo)) this._listeningTo = void 0;
return this;
}

Expand All @@ -223,28 +168,105 @@

// Implement fancy features of the Events API such as multiple event
// names `"change blur"` and jQuery-style event maps `{change: action}`
// in terms of the existing API.
var eventsApi = function(obj, action, name, rest) {
if (!name) return true;

// Handle event maps.
if (typeof name === 'object') {
for (var key in name) {
obj[action].apply(obj, [key, name[key]].concat(rest));
// in terms of the existing API. Consider this just a special purpose
// reduce function.
var eventsApi = function(api, events, name, callback, context, ctx) {
var i = 0, names, length;
if (name && typeof name === 'object') {
// Handle event maps.
for (names = _.keys(name), length = names.length; i < length; i++) {
events = api(events, names[i], name[names[i]], context, ctx);
}
return false;
} else if (name && eventSplitter.test(name)) {
// Handle space separated event names.
for (names = name.split(eventSplitter), length = names.length; i < length; i++) {
events = api(events, names[i], callback, context, ctx);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the redundancy in these two can be addressed but it can be done in a follow up

}
} else {
events = api(events, name, callback, context, ctx);
}
return events;
};

// Handles actually adding the event handler.
var onApi = function(events, name, callback, context, ctx) {
if (callback) {
var handlers = events[name] || [];
events[name] = handlers.concat({callback: callback, context: context, ctx: context || ctx});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason you didn't want to use push again, I saw the issue but didnt read it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agh. I think we've decided to slice in #trigger instead of concat here. I'll need to update this.

}
return events;
};

// Handles removing any event handlers that are no longer wanted.
var offApi = function(events, name, callback, context) {
// Remove all callbacks for all events.
if (!events || !name && !context && !callback) return;

var names = name ? [name] : _.keys(events);
for (var i = 0, length = names.length; i < length; i++) {
name = names[i];

// Bail out if there are no events stored.
var handlers = events[name];
if (!handlers) continue;

// Find any remaining events.
var remaining = [];
if (callback || context) {
for (var j = 0, k = handlers.length; j < k; j++) {
var handler = handlers[j];
if (
callback && callback !== handler.callback &&
callback !== handler.callback._callback ||
context && context !== handler.context
) {
remaining.push(handler);
}
}
}

// Handle space separated event names.
if (eventSplitter.test(name)) {
var names = name.split(eventSplitter);
for (var i = 0, length = names.length; i < length; i++) {
obj[action].apply(obj, [names[i]].concat(rest));
// Replace events if there are any remaining. Otherwise, clean up.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: double space

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😛

if (remaining.length) {
events[name] = remaining;
} else {
delete events[name];
}
return false;
}
return _.isEmpty(events) ? void 0 : events;
};

// When triggering with an `{event: value}` object, `value` should be
// prepended to the arguments passed onto the event callbacks.
var triggerSentinel = {};

// Handles triggering the appropriate event callbacks.
var triggerApi = function(obj, name, sentinel, args) {
if (obj._events) {
// Prepend `sentinel` onto args when trigger was called with
// an object.
if (sentinel !== triggerSentinel) args = [sentinel].concat(args);

var events = obj._events[name];
var allEvents = obj._events.all;
if (events) triggerEvents(events, args);
if (allEvents) triggerEvents(allEvents, [name].concat(args));
}
return obj;
};

return true;
// Reduces the event callbacks into a map of `{event: onceWrapper}`.
// `offer` is used to unbind the `onceWrapper` after it as been called.
var onceMap = function(name, callback, offer) {
return eventsApi(function(map, name, callback, offer) {
if (callback) {
var once = map[name] = _.once(function() {
offer(name, once);
callback.apply(this, arguments);
});
once._callback = callback;
}
return map;
}, {}, name, callback, offer);
};

// A difficult-to-believe, but optimized internal dispatch function for
Expand Down
2 changes: 1 addition & 1 deletion test/collection.js
Original file line number Diff line number Diff line change
Expand Up @@ -1329,7 +1329,7 @@
equal(this._byId[model.id], void 0);
equal(this._byId[model.cid], void 0);
equal(model.collection, void 0);
equal(model._events.all, void 0);
equal(model._events, void 0);
}

});
Expand Down
Loading