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

Conversation

anandthakker
Copy link
Contributor

Closes #3086

@anandthakker
Copy link
Contributor Author

@jfirebaugh This should fix it. Confirmed it with a quick check using jsfiddle example linked in the issue.

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.

@jfirebaugh
Copy link
Contributor

Another issue with #2952 is that it can leak WebWorker message event bindings: the binding added in the Actor constructor is never removed. It looks like if the last Map is removed than the workers will get shut down, but as long as there is at least one Map, then all the Actors ever created will stay live.

Can you add a fix for that to this PR as well?

@jfirebaugh
Copy link
Contributor

Another issue: the blob URLs that webworkify creates aren't revoked; if you repeatedly create and remove maps, so that the worker pool is drained and refilled, they leak.

@anandthakker
Copy link
Contributor Author

Another issue with #2952 is that it can leak WebWorker message event bindings: the binding added in the Actor constructor is never removed. It looks like if the last Map is removed than the workers will get shut down, but as long as there is at least one Map, then all the Actors ever created will stay live.

I think this predates 2952, but yep I can add a fix for this.

@anandthakker
Copy link
Contributor Author

I think this predates 2952

Ah, actually it probably doesn't predate 2952, since it's probably a pretty safe assumption that, before the worker pool, the message listeners were being removed in WebWorker#terminate. My mistake.

@anandthakker
Copy link
Contributor Author

@jfirebaugh included fixes to dispose of event listeners and revoke object URLs.

@lucaswoj also tried to clarify the code in Actor to avoid the need for an inline comment; but at the same time, I thought it did make sense to include standard jsdoc for Actor#send.

@lucaswoj lucaswoj merged commit 597a1a8 into master Sep 12, 2016
@lucaswoj lucaswoj deleted the fix-worker-messages branch September 12, 2016 21:22
@lucaswoj
Copy link
Contributor

lucaswoj commented Sep 12, 2016

This is ready to 🚢. We need to fix this bug before the next release. I'm also keen on simplifying our worker architecture in a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants