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 bug causing worker-initiated messages to be broadcast to every map instance #3133

Merged
merged 5 commits into from
Sep 12, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion js/source/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,16 @@ util.extend(Worker.prototype, {
getLayers: function () { return this.layers[mapId]; }.bind(this),
getLayerFamilies: function () { return this.layerFamilies[mapId]; }.bind(this)
};
this.workerSources[mapId][type] = new this.workerSourceTypes[type](this.actor, layers);

// use a wrapped actor so that we can attach a target mapId param
// to any messages invoked by the WorkerSource
var actor = {
send: function (type, data, callback, buffers) {
this.actor.send(type, data, callback, buffers, mapId);
}.bind(this)
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like we're fighting our own architecture with this fake Actor. Can you think of a way to refactor & simplify? Worker and Actor are so tightly coupled it might make sense to merge them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think merging Worker and Actor is probably beyond the scope of this bug fix, but I certainly agree your broader point over in #3034.

I was trying to keep the scope of this change as limited as possible, but maybe one way to simplify this a bit would be to replace WorkerSource's (and WorkerTile#parse's) actor: Actor parameter with just a send: (type, data, callback)=>void parameter.


this.workerSources[mapId][type] = new this.workerSourceTypes[type](actor, layers);
}

return this.workerSources[mapId][type];
Expand Down
38 changes: 31 additions & 7 deletions js/util/actor.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,30 +23,56 @@ function Actor(target, parent, mapId) {
this.target.addEventListener('message', this.receive, false);
}

/**
* Sends a message from a main-thread map to a Worker or from a Worker back to
* a main-thread map instance.
*
* @param {string} type The name of the target method to invoke or '[source-type].name' for a method on a WorkerSource.
* @param {object} data
* @param {Function} [callback]
* @param {Array} [buffers] A list of buffers to "transfer" (see https://developer.mozilla.org/en-US/docs/Web/API/Transferable)
* @param {string} [targetMapId] A particular mapId to which to send this message.
* @private
*/
Actor.prototype.send = function(type, data, callback, buffers, targetMapId) {
var id = callback ? this.mapId + ':' + this.callbackID++ : null;
if (callback) this.callbacks[id] = callback;
this.postMessage({
targetMapId: targetMapId,
sourceMapId: this.mapId,
type: type,
id: String(id),
data: data
}, buffers);
};

Actor.prototype.receive = function(message) {
var data = message.data,
id = data.id,
callback;

if (data.targetMapId && this.mapId !== data.targetMapId)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there's a way you can make this code more self-documenting such that the comment is unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Will do


if (data.type === '<response>') {
callback = this.callbacks[data.id];
delete this.callbacks[data.id];
if (callback) callback(data.error || null, data.data);
} else if (typeof data.id !== 'undefined' && this.parent[data.type]) {
// data.type == 'load tile', 'remove tile', etc.
this.parent[data.type](data.mapId, data.data, done.bind(this));
this.parent[data.type](data.sourceMapId, data.data, done.bind(this));
} else if (typeof data.id !== 'undefined' && this.parent.getWorkerSource) {
// data.type == sourcetype.method
var keys = data.type.split('.');
var workerSource = this.parent.getWorkerSource(data.mapId, keys[0]);
var workerSource = this.parent.getWorkerSource(data.sourceMapId, keys[0]);
workerSource[keys[1]](data.data, done.bind(this));
} else {
this.parent[data.type](data.data);
}

function done(err, data, buffers) {
this.postMessage({
mapId: this.mapId,
sourceMapId: this.mapId,
type: '<response>',
id: String(id),
error: err ? String(err) : null,
Expand All @@ -55,10 +81,8 @@ Actor.prototype.receive = function(message) {
}
};

Actor.prototype.send = function(type, data, callback, buffers) {
var id = callback ? this.mapId + ':' + this.callbackID++ : null;
if (callback) this.callbacks[id] = callback;
this.postMessage({ mapId: this.mapId, type: type, id: String(id), data: data }, buffers);
Actor.prototype.remove = function () {
this.target.removeEventListener('message', this.receive, false);
};

/**
Expand Down
3 changes: 2 additions & 1 deletion js/util/dispatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,9 @@ Dispatcher.prototype = {
},

remove: function() {
this.workerPool.release(this.id);
this.actors.forEach(function (actor) { actor.remove(); });
this.actors = [];
this.workerPool.release(this.id);
}
};

6 changes: 6 additions & 0 deletions js/util/web_worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ function MessageBus(addListeners, postListeners) {
addListeners.push(callback);
}
},
removeEventListener: function(event, callback) {
var i = addListeners.indexOf(callback);
if (i >= 0) {
addListeners.splice(i, 1);
}
},
postMessage: function(data) {
setImmediate(function() {
for (var i = 0; i < postListeners.length; i++) {
Expand Down
2 changes: 2 additions & 0 deletions js/util/window.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ function restore() {
window.XMLHttpRequest = window.server.xhr;
};

window.URL.revokeObjectURL = function () {};

window.restore = restore;

return window;
Expand Down
6 changes: 5 additions & 1 deletion js/util/worker_pool.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

var assert = require('assert');
var WebWorker = require('./web_worker');
var URL = require('./window').URL;

module.exports = WorkerPool;

Expand Down Expand Up @@ -35,7 +36,10 @@ WorkerPool.prototype = {
release: function (mapId) {
delete this.active[mapId];
if (Object.keys(this.active).length === 0) {
this.workers.forEach(function (w) { w.terminate(); });
this.workers.forEach(function (w) {
URL.revokeObjectURL(w.objectURL);
w.terminate();
});
this.workers = null;
}
}
Expand Down
20 changes: 20 additions & 0 deletions test/js/source/worker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,23 @@ test('update layers isolates different instances\' data', function(t) {

t.end();
});

test('worker source messages dispatched to the correct map instance', function(t) {
var worker = new Worker(_self);

worker.actor.send = function (type, data, callback, buffers, mapId) {
t.equal(type, 'main thread task');
t.equal(mapId, 999);
t.end();
};

_self.registerWorkerSource('test', function(actor) {
this.loadTile = function() {
// we expect the map id to get appended in the call to the "real"
// actor.send()
actor.send('main thread task', {}, function () {}, null);
};
});

worker['load tile'](999, {type: 'test'});
});
37 changes: 37 additions & 0 deletions test/js/util/actor.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,42 @@ test('Actor', function (t) {
});
});

t.test('targets worker-initiated messages to correct map instance', function (t) {
var workerActor;

var WebWorker = proxyquire('../../../js/util/web_worker', {
'../source/worker': function Worker(self) {
this.self = self;
this.actor = workerActor = new Actor(self, this);
}
});
var worker = new WebWorker();

new Actor(worker, {
test: function () { t.end(); }
}, 'map-1');
new Actor(worker, {
test: function () {
t.fail();
t.end();
}
}, 'map-2');

workerActor.send('test', {}, function () {}, null, 'map-1');
});

t.test('#remove unbinds event listener', function (t) {
var actor = new Actor({
addEventListener: function (type, callback, useCapture) {
this._addEventListenerArgs = [type, callback, useCapture];
},
removeEventListener: function (type, callback, useCapture) {
t.same([type, callback, useCapture], this._addEventListenerArgs, 'listener removed');
t.end();
}
}, {}, null);
actor.remove();
});

t.end();
});
19 changes: 18 additions & 1 deletion test/js/util/dispatcher.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ test('Dispatcher', function (t) {
t.end();
});

test('creates Actors with unique map id', function (t) {
t.test('creates Actors with unique map id', function (t) {
var Dispatcher = proxyquire('../../../js/util/dispatcher', {'./actor': Actor });
var WorkerPool = proxyquire('../../../js/util/worker_pool', {
'../mapbox-gl': { workerCount: 1 }
Expand All @@ -45,6 +45,23 @@ test('Dispatcher', function (t) {
t.end();
});

t.test('#remove destroys actors', function (t) {
var Dispatcher = proxyquire('../../../js/util/dispatcher', {'./actor': Actor });
var actorsRemoved = [];
function Actor() {
this.remove = function() { actorsRemoved.push(this); };
}
var WorkerPool = proxyquire('../../../js/util/worker_pool', {
'../mapbox-gl': { workerCount: 4 }
});

var workerPool = new WorkerPool();
var dispatcher = new Dispatcher(workerPool, {});
dispatcher.remove();
t.equal(actorsRemoved.length, 4);
t.end();
});

t.end();
});

20 changes: 14 additions & 6 deletions test/js/util/worker_pool.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,34 @@ test('WorkerPool', function (t) {
});

t.test('#release', function (t) {
var workersTerminated = 0;
var objectUrlsRevoked = {};
var WorkerPool = proxyquire('../../../js/util/worker_pool', {
'../mapbox-gl': { workerCount: 4 }
'../mapbox-gl': { workerCount: 4 },
'./window': {
URL: {
revokeObjectURL: function (url) { objectUrlsRevoked[url] = true; }
}
}
});

var pool = new WorkerPool();
pool.acquire('map-1');
var workers = pool.acquire('map-2');
var terminated = 0;
workers.forEach(function (w) {
w.terminate = function () { terminated += 1; };
workers.forEach(function (w, i) {
w.terminate = function () { workersTerminated += 1; };
w.objectURL = 'blob:worker-' + i;
});

pool.release('map-2');
t.comment('keeps workers if a dispatcher is still active');
t.equal(terminated, 0);
t.equal(workersTerminated, 0);
t.ok(pool.workers.length > 0);

t.comment('terminates workers if no dispatchers are active');
pool.release('map-1');
t.equal(terminated, 4);
t.equal(workersTerminated, 4);
t.equal(Object.keys(objectUrlsRevoked).length, 4);
t.notOk(pool.workers);

t.end();
Expand Down