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

Add support for AsyncResource #1472

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Sebmaster

Sebmaster commented Oct 27, 2017

I kinda hijacked the domain architecture to implement AsyncResource, but it makes the changes very consistent and keeps the context stuff transparent to the rest of the code.

Fixes #1403.

@Sebmaster

This comment has been minimized.

Show comment
Hide comment
@Sebmaster

Sebmaster Oct 27, 2017

I sadly can't quite figure out why this is breaking some tests.

A simple old = cb; cb = function(){return old.apply(this, arguments);} in the bindContext function breaks everything. Something's special about this original function object in some cases.

Is there like some hidden property bluebird attaches to them?

Sebmaster commented Oct 27, 2017

I sadly can't quite figure out why this is breaking some tests.

A simple old = cb; cb = function(){return old.apply(this, arguments);} in the bindContext function breaks everything. Something's special about this original function object in some cases.

Is there like some hidden property bluebird attaches to them?

@Sebmaster

This comment has been minimized.

Show comment
Hide comment
@Sebmaster

Sebmaster Oct 27, 2017

Alright, found the specific compare to reflectHandler and fixed that.

There's one thing which is kinda "unimplemented" which is emitDestroy, but I don't think I understand the lifecycle of the objects enough to determine where we'd need to put that.

Sebmaster commented Oct 27, 2017

Alright, found the specific compare to reflectHandler and fixed that.

There's one thing which is kinda "unimplemented" which is emitDestroy, but I don't think I understand the lifecycle of the objects enough to determine where we'd need to put that.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr
Collaborator

benjamingr commented Oct 28, 2017

Show outdated Hide outdated src/promise.js Outdated
return ret;
};
if (util.nodeSupportsAsyncResource) {
var AsyncResource = require("async_hooks").AsyncResource;

This comment has been minimized.

@jkrems

jkrems Nov 4, 2017

Is this stripped somehow in the browser build? E.g. will this expression make it into js/browser/bluebird.js? If so, it might cause problems while bundling in apps using bluebird. Maybe we'd want to put this logic into a completely separate file that's "guarded" by a file-level browser override so it's excluded from bundling? E.g. something like this:

// src/getContext.browser.js, gets loaded directly in browser environments
module.exports = function getContext() { return {}; }

// src/getContext.js, gets loaded in not-definitely-browser environments
var getFallbackContext = require('./getContext.browser.js');
var getContext;
// [...]

// src/promise.js
var getContext = require('./getContext');
@jkrems

jkrems Nov 4, 2017

Is this stripped somehow in the browser build? E.g. will this expression make it into js/browser/bluebird.js? If so, it might cause problems while bundling in apps using bluebird. Maybe we'd want to put this logic into a completely separate file that's "guarded" by a file-level browser override so it's excluded from bundling? E.g. something like this:

// src/getContext.browser.js, gets loaded directly in browser environments
module.exports = function getContext() { return {}; }

// src/getContext.js, gets loaded in not-definitely-browser environments
var getFallbackContext = require('./getContext.browser.js');
var getContext;
// [...]

// src/promise.js
var getContext = require('./getContext');

This comment has been minimized.

@Sebmaster

Sebmaster Nov 4, 2017

Yes, see the exclude in the browserify instructions, require will just return an empty object in that case (and even that won't be executed due to the isNode check before).

@Sebmaster

Sebmaster Nov 4, 2017

Yes, see the exclude in the browserify instructions, require will just return an empty object in that case (and even that won't be executed due to the isNode check before).

This comment has been minimized.

@jkrems

jkrems Nov 4, 2017

Ah, gotcha. It compiles to _dereq_('async_hooks') which just never gets executed. 👍

@jkrems

jkrems Nov 4, 2017

Ah, gotcha. It compiles to _dereq_('async_hooks') which just never gets executed. 👍

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Nov 9, 2017

Collaborator

Another attempt to solicit feedback from @addaleax about this

Collaborator

benjamingr commented Nov 9, 2017

Another attempt to solicit feedback from @addaleax about this

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2017

@benjamingr Sorry, the reason I haven’t commented so far is: The async_hooks API sucks for these use cases at this point, and we’re going to fix that, but that hasn’t happened yet. Every resource needs a destroy hook, otherwise everything is just a memory leak, and for JS resources that would be done through GC tracking which isn’t there yet.

So, uh, I have this on my radar… if anybody wants to make the necessary improvements inside of Node themselves, please go for it, and know that I’m available (same handle on twitter/irc) for any questions, help, guidance, whatever :)

addaleax commented Nov 9, 2017

@benjamingr Sorry, the reason I haven’t commented so far is: The async_hooks API sucks for these use cases at this point, and we’re going to fix that, but that hasn’t happened yet. Every resource needs a destroy hook, otherwise everything is just a memory leak, and for JS resources that would be done through GC tracking which isn’t there yet.

So, uh, I have this on my radar… if anybody wants to make the necessary improvements inside of Node themselves, please go for it, and know that I’m available (same handle on twitter/irc) for any questions, help, guidance, whatever :)

@Sebmaster

This comment has been minimized.

Show comment
Hide comment
@Sebmaster

Sebmaster Nov 9, 2017

@addaleax nodejs/node#16153 would be the ticket for that, I assume?

@benjamingr Do you want to leave this open until the changes in node land?

Sebmaster commented Nov 9, 2017

@addaleax nodejs/node#16153 would be the ticket for that, I assume?

@benjamingr Do you want to leave this open until the changes in node land?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2017

@Sebmaster Yeah, thanks for looking that up. 😄 I’ve labeled that as a good first issue.

Also, I guess it doesn’t hurt to leave this open or even merge this without GC support – async_hooks is experimental for a reason, and this is just plainly a bug in core.

addaleax commented Nov 9, 2017

@Sebmaster Yeah, thanks for looking that up. 😄 I’ve labeled that as a good first issue.

Also, I guess it doesn’t hurt to leave this open or even merge this without GC support – async_hooks is experimental for a reason, and this is just plainly a bug in core.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2017

As for the actual diff here, I think there might be too much bluebird-specific stuff in it for me to grok easily?

addaleax commented Nov 9, 2017

As for the actual diff here, I think there might be too much bluebird-specific stuff in it for me to grok easily?

@Sebmaster

This comment has been minimized.

Show comment
Hide comment
@Sebmaster

Sebmaster Nov 9, 2017

or even merge this without GC support – async_hooks is experimental for a reason, and this is just plainly a bug in core.

In that case we'd need at least a config option to enable AsyncResource, I think (and disable by default). Can't start leaking asyncIds on a random minor bluebird update.

Sebmaster commented Nov 9, 2017

or even merge this without GC support – async_hooks is experimental for a reason, and this is just plainly a bug in core.

In that case we'd need at least a config option to enable AsyncResource, I think (and disable by default). Can't start leaking asyncIds on a random minor bluebird update.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Nov 10, 2017

Collaborator

Thanks for chiming in @addaleax

I'd think we'd rather explicitly not support async_hooks than give users something that can leak memory at this point.

I'm ok with leaving this open until this gets fixed in core

Collaborator

benjamingr commented Nov 10, 2017

Thanks for chiming in @addaleax

I'd think we'd rather explicitly not support async_hooks than give users something that can leak memory at this point.

I'm ok with leaving this open until this gets fixed in core

fixup! only allow node 9.3.0 and up
Previous versions of node did not emit a destroy event on gc, leading
to leaked async ids.
@Sebmaster

This comment has been minimized.

Show comment
Hide comment
@Sebmaster

Sebmaster Dec 12, 2017

New node release is out and has support for automatic destroy call on GC, so limiting async_hooks support to 9.3.0 and up. Should be ready now.

Sebmaster commented Dec 12, 2017

New node release is out and has support for automatic destroy call on GC, so limiting async_hooks support to 9.3.0 and up. Should be ready now.

Show outdated Hide outdated src/promise.js Outdated
@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen Dec 15, 2017

@addaleax @Sebmaster master do we want to expose an emitPromiseResolve for the promiseResolve hook?

AndreasMadsen commented Dec 15, 2017

@addaleax @Sebmaster master do we want to expose an emitPromiseResolve for the promiseResolve hook?

@suguru03

This comment has been minimized.

Show comment
Hide comment
@suguru03

suguru03 Dec 15, 2017

Contributor

Excuse me for cutting in.
I realized a performance issue.
I tested ./bench doxbee, it was approximately two times slower than before.
I just started learning async_hooks and AsyncResource, so I'm not sure if it is required or not. If it is not, I would like to suggest using Bluebird.config and the default is false.

file                                       time(ms)  memory(MB)
callbacks-baseline.js                           214       27.03
promises-suguru03-aigle.js                      304       48.38
callbacks-suguru03-neo-async-waterfall.js       312       35.63
promises-bluebird-generator.js                  328       37.34
promises-cujojs-when.js                         511       66.89
promises-tildeio-rsvp.js                        528       81.32
promises-lvivski-davy.js                        593       91.18
promises-then-promise.js                        597       72.99
promises-native-async-await.js                  631      108.68
generators-tj-co.js                             660       80.39
promises-bluebird.js                            681       84.18
promises-ecmascript6-native.js                  696      104.88
callbacks-caolan-async-waterfall.js             788       80.94
promises-dfilatov-vow.js                        904      134.86
promises-obvious-kew.js                        1205      115.54
promises-calvinmetcalf-lie.js                  1228      116.02
streamline-generators.js                       1281       82.92
promises-medikoo-deferred.js                   1392       96.83
observables-pozadi-kefir.js                    1669      143.13
streamline-callbacks.js                        2010      105.96
observables-Reactive-Extensions-RxJS.js        3293      187.86
promises-kriskowal-q.js                       10433      305.79
observables-caolan-highland.js                14689      434.57
observables-baconjs-bacon.js.js               30249      751.06
Contributor

suguru03 commented Dec 15, 2017

Excuse me for cutting in.
I realized a performance issue.
I tested ./bench doxbee, it was approximately two times slower than before.
I just started learning async_hooks and AsyncResource, so I'm not sure if it is required or not. If it is not, I would like to suggest using Bluebird.config and the default is false.

file                                       time(ms)  memory(MB)
callbacks-baseline.js                           214       27.03
promises-suguru03-aigle.js                      304       48.38
callbacks-suguru03-neo-async-waterfall.js       312       35.63
promises-bluebird-generator.js                  328       37.34
promises-cujojs-when.js                         511       66.89
promises-tildeio-rsvp.js                        528       81.32
promises-lvivski-davy.js                        593       91.18
promises-then-promise.js                        597       72.99
promises-native-async-await.js                  631      108.68
generators-tj-co.js                             660       80.39
promises-bluebird.js                            681       84.18
promises-ecmascript6-native.js                  696      104.88
callbacks-caolan-async-waterfall.js             788       80.94
promises-dfilatov-vow.js                        904      134.86
promises-obvious-kew.js                        1205      115.54
promises-calvinmetcalf-lie.js                  1228      116.02
streamline-generators.js                       1281       82.92
promises-medikoo-deferred.js                   1392       96.83
observables-pozadi-kefir.js                    1669      143.13
streamline-callbacks.js                        2010      105.96
observables-Reactive-Extensions-RxJS.js        3293      187.86
promises-kriskowal-q.js                       10433      305.79
observables-caolan-highland.js                14689      434.57
observables-baconjs-bacon.js.js               30249      751.06
@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen Dec 15, 2017

@Sebmaster @suguru03 This makes sense. AsyncResource listens on the garbage collector which is a rather slow operation.

@addaleax We should consider only calling registerDestroyHook if there are actual listeners, that should give a big speedup in the default case. I'm not a huge fan of this, as it makes late enabling of async_hooks a little odd. Although, we kinda have the same situation for native promises – also not a fan of that :p

AndreasMadsen commented Dec 15, 2017

@Sebmaster @suguru03 This makes sense. AsyncResource listens on the garbage collector which is a rather slow operation.

@addaleax We should consider only calling registerDestroyHook if there are actual listeners, that should give a big speedup in the default case. I'm not a huge fan of this, as it makes late enabling of async_hooks a little odd. Although, we kinda have the same situation for native promises – also not a fan of that :p

@Sebmaster

This comment has been minimized.

Show comment
Hide comment
@Sebmaster

Sebmaster Jan 12, 2018

Alright, fixed all the feedback, added a config option for enabling AsyncResource.

@AndreasMadsen Sure we could emit the event, however it seems like looping through the proper AsyncResource is kinda difficult right now, since it's not really promise-instance-specific, but just some param which gets weaved through the API.

I think there might be a behaviour difference here now. Native promises generate a single AsyncResource for the whole promise lifecycle(?), while this current implementation creates one AsyncResource per .then call, to use the domain infrastructure. Maybe that should change...

Sebmaster commented Jan 12, 2018

Alright, fixed all the feedback, added a config option for enabling AsyncResource.

@AndreasMadsen Sure we could emit the event, however it seems like looping through the proper AsyncResource is kinda difficult right now, since it's not really promise-instance-specific, but just some param which gets weaved through the API.

I think there might be a behaviour difference here now. Native promises generate a single AsyncResource for the whole promise lifecycle(?), while this current implementation creates one AsyncResource per .then call, to use the domain infrastructure. Maybe that should change...

@kjin

This comment has been minimized.

Show comment
Hide comment
@kjin

kjin Sep 25, 2018

Hi @Sebmaster -- any update on this PR? It's been almost a year since it has first opened, so I'm sure some of the concerns about it landing (with regards to the async_hooks API) might have been alleviated in the meantime. On the other hand, if there are still concrete things that still need to be done in Node core, maybe it would be good to mention what those things are now.

kjin commented Sep 25, 2018

Hi @Sebmaster -- any update on this PR? It's been almost a year since it has first opened, so I'm sure some of the concerns about it landing (with regards to the async_hooks API) might have been alleviated in the meantime. On the other hand, if there are still concrete things that still need to be done in Node core, maybe it would be good to mention what those things are now.

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