meteor / promise

ES6 Promise polyfill with Fiber support
MIT License
63 stars 15 forks source link

Don't double-wrap callbacks. #11

Closed kentonv closed 8 years ago

kentonv commented 8 years ago

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: https://github.com/meteor/meteor/issues/7747

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

Fixes #10

kentonv commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Great! When can we expect to see that?

kentonv commented 8 years ago

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

abernix commented 8 years ago

@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 commented 8 years ago

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

deanrad commented 6 years ago

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