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

Don't double-wrap callbacks. #11

Merged
merged 1 commit into from Sep 6, 2016

Conversation

Projects
None yet
4 participants
@kentonv
Copy link
Contributor

kentonv commented Sep 5, 2016

When using Promises, a callback passed to .then() will sometimes pass through multiple .then() calls. If we aren't careful, we may end up wrapping a callback multiple times, resulting in a callback that creates a fiber which immediately creates another fiber and so on.

This is especially bad because in this chain situation of fibers-creating-fibers, none of the fibers complete before others are created, thus they force expansion of the fiber pool.

This, in turn, runs into a v8 bug in which each fiber created makes fiber-switching permanently slower, due to use of a linked list. See: meteor/meteor#7747

This change simply adds a field to the wrapped callbacks that marks them as not needing further wrapping.

Fixes #10

Don't double-wrap callbacks.
When using Promises, a callback passed to .then() will sometimes pass through multiple .then() calls. If we aren't careful, we may end up wrapping a callback multiple times, resulting in a callback that creates a fiber which immediately creates another fiber and so on.

This is especially bad because in this chain situation of fibers-creating-fibers, none of the fibers complete before others are created, thus they force expansion of the fiber pool.

This, in turn, runs into a v8 bug in which each fiber created makes fiber-switching permanently slower, due to use of a linked list. See:

    meteor/meteor#7747

This change simply adds a field to the wrapped callbacks that marks them as not needing further wrapping.

Fixes #10
@kentonv

This comment has been minimized.

Copy link
Contributor

kentonv commented Sep 5, 2016

I would appreciate if this could either rapidly find its way into a Meteor point release or if you could instruct me on how to force Meteor to use this patch in our build. This bug is causing repeated production outages, and although I have hotfixed it for now by editing the code in-place on the server, that's obviously not a sustainable solution.

Thanks!

@benjamn benjamn merged commit d68469e into meteor:master Sep 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamn

This comment has been minimized.

Copy link
Member

benjamn commented Sep 6, 2016

Fortunately, this should only require a minor version bump for the Meteor promise package, which can then be released independently, and Meteor 1.4 users will be able to run meteor update promise, thanks to version unpinning.

@kentonv

This comment has been minimized.

Copy link
Contributor

kentonv commented Sep 6, 2016

Great! When can we expect to see that?

@kentonv

This comment has been minimized.

Copy link
Contributor

kentonv commented Sep 8, 2016

We usually cut releases on Fridays. Will you have a chance to publish this before then?

benjamn added a commit to meteor/meteor that referenced this pull request Sep 9, 2016

Update `meteor-promise` npm package to v0.7.4.
Fixes a bug where .then callbacks could be wrapped multiple times:
meteor/promise#11
@abernix

This comment has been minimized.

Copy link
Member

abernix commented Sep 16, 2016

@kentonv Not sure if you got a chance to try this last Friday, but the Meteor promise package should be updated with this pull now. If you haven't tried it already, try updating your promise package to 0.8.7 – you should be able to just do meteor update promise!

@kentonv

This comment has been minimized.

Copy link
Contributor

kentonv commented Sep 17, 2016

@abernix Yep, I caught it last week and it made it into our release. Thanks!

@deanius

This comment has been minimized.

Copy link

deanius commented May 1, 2018

Nice catch @kentonv , and great write-up on Sandstorm

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