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

async_wrap,src: promise hook integration #13000

Closed
wants to merge 16 commits into
base: master
from

Conversation

@matthewloring
Contributor

matthewloring commented May 12, 2017

This change provides unified tracking of asynchronous promise lifecycles
for both domains and async hooks.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

async_wrap,src

async_wrap,src: promise hook integration
This change provides unified tracking of asynchronous promise lifecycles
for both domains and async hooks.
@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring May 12, 2017

Contributor

Here is the work I have so far on promise integrations for domains/async hooks. There are a few outstanding things I wanted to discuss:

  1. Currently promises do not have an internal field slot that can be used by BaseObject. To get around this, we create a separate object stored on the promise that can be wrapped and delegate domain lookups to that object. Maybe @gsathya has a better approach for this or could comment on the feasibility of having an internal field added to the promise object template.

  2. It would be good to surface additional information that promise hooks makes available through async hooks. Specifically, promise hooks gives information about parent-child relationships when promises are constructed as part of a chain. It would be nice to pass metadata to the async hook callbacks that could relay this (or maybe there is already a pattern in place for making this available). Also, some diagnostic tools (specifically for resource tracking) are interested in knowing when promises resolve in addition to the "before" and "after" events. Maybe we could consider adding a resolution async hook event (potentially this could be used by other resource types other than just promises). I wrote up some more thoughts on this here.

  3. It would be good to get a review from someone with more understanding of test-async-wrap=-getasyncid.js to figure out the right way of exposing the promise wrap object to JS.

Contributor

matthewloring commented May 12, 2017

Here is the work I have so far on promise integrations for domains/async hooks. There are a few outstanding things I wanted to discuss:

  1. Currently promises do not have an internal field slot that can be used by BaseObject. To get around this, we create a separate object stored on the promise that can be wrapped and delegate domain lookups to that object. Maybe @gsathya has a better approach for this or could comment on the feasibility of having an internal field added to the promise object template.

  2. It would be good to surface additional information that promise hooks makes available through async hooks. Specifically, promise hooks gives information about parent-child relationships when promises are constructed as part of a chain. It would be nice to pass metadata to the async hook callbacks that could relay this (or maybe there is already a pattern in place for making this available). Also, some diagnostic tools (specifically for resource tracking) are interested in knowing when promises resolve in addition to the "before" and "after" events. Maybe we could consider adding a resolution async hook event (potentially this could be used by other resource types other than just promises). I wrote up some more thoughts on this here.

  3. It would be good to get a review from someone with more understanding of test-async-wrap=-getasyncid.js to figure out the right way of exposing the promise wrap object to JS.

@matthewloring matthewloring referenced this pull request May 12, 2017

Closed

[async_hooks] tracking issue #29

23 of 27 tasks complete
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 May 12, 2017

Member

My quick thoughts on (2.) is that it would probably be better have the returned Resource Object be it's own abstracted information object rather than the Promise itself. It can still have the actual Promise as a property or getter if necessary.

Member

Fishrock123 commented May 12, 2017

My quick thoughts on (2.) is that it would probably be better have the returned Resource Object be it's own abstracted information object rather than the Promise itself. It can still have the actual Promise as a property or getter if necessary.

Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/node.cc
@gsathya

This comment has been minimized.

Show comment
Hide comment
@gsathya

gsathya May 15, 2017

Member

Maybe @gsathya has a better approach for this or could comment on the feasibility of having an internal field added to the promise object template.

The current approach seems fine to me. You can use a private symbol if you don't think this should be exposed.

LGTM. Thanks for implementing this!

Member

gsathya commented May 15, 2017

Maybe @gsathya has a better approach for this or could comment on the feasibility of having an internal field added to the promise object template.

The current approach seems fine to me. You can use a private symbol if you don't think this should be exposed.

LGTM. Thanks for implementing this!

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav May 16, 2017

Member

@matthewloring

Specifically, promise hooks gives information about parent-child relationships when promises are constructed as part of a chain. It would be nice to pass metadata to the async hook callbacks that could relay this (or maybe there is already a pattern in place for making this available).

I think trigger_id is meant to serve as the parent context ID. It's made available to init hooks here and seems that it should be available as the second parameter to before hooks (see e.g. here); but the before hook implementation of trigger_id seems incomplete here.

Member

joshgav commented May 16, 2017

@matthewloring

Specifically, promise hooks gives information about parent-child relationships when promises are constructed as part of a chain. It would be nice to pass metadata to the async hook callbacks that could relay this (or maybe there is already a pattern in place for making this available).

I think trigger_id is meant to serve as the parent context ID. It's made available to init hooks here and seems that it should be available as the second parameter to before hooks (see e.g. here); but the before hook implementation of trigger_id seems incomplete here.

@addaleax

Looks mostly good. I’d have a preference for https://github.com/nodejs/node/pull/13000/files#r117075597 to be used here, but I can also PR that separately if you prefer.

Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/node.cc
Show outdated Hide outdated src/async-wrap.cc
@trevnorris

Great work here. Have a few things, and let me know if you'd like an assist.

Show outdated Hide outdated src/async-wrap.cc
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(wrap->env());
FatalException(wrap->env()->isolate(), try_catch);
return false;

This comment has been minimized.

@trevnorris

trevnorris May 19, 2017

Contributor

FatalException() always exits the application in this case because ClearFatalException() removed all unhandledException callbacks. So I'll assume this return is so the compiler doesn't emit a warning.

I decided this would be the safest course of action when first implementing AsyncWrap because it was difficult to properly clean up if an async hook threw. It was difficult enough to properly cleanup if a user's callback threw (see process._fatalException() in lib/internal/bootstrap_node.js). If we can properly show it's safe to recover from an async hook throwing then I'm all for changing this behavior.

@trevnorris

trevnorris May 19, 2017

Contributor

FatalException() always exits the application in this case because ClearFatalException() removed all unhandledException callbacks. So I'll assume this return is so the compiler doesn't emit a warning.

I decided this would be the safest course of action when first implementing AsyncWrap because it was difficult to properly clean up if an async hook threw. It was difficult enough to properly cleanup if a user's callback threw (see process._fatalException() in lib/internal/bootstrap_node.js). If we can properly show it's safe to recover from an async hook throwing then I'm all for changing this behavior.

This comment has been minimized.

@matthewloring

matthewloring May 24, 2017

Contributor

That makes sense, maybe we can look into that more in a follow on pr.

@matthewloring

matthewloring May 24, 2017

Contributor

That makes sense, maybe we can look into that more in a follow on pr.

Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/async-wrap.cc
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 21, 2017

Member

@matthewloring I went ahead and pushed fixes for some of mine and Trevor’s review comments to this PR, feel free to remove anything that you disagree with.

Edit: Just so you know, I’m especially unsure about the last commit. I think it makes sense, but I’m not 100 % sure.

@trevnorris @AndreasMadsen Please take a look, and it would be great if you could pinpoint what you think should definitely be changed before this is ready to land.

CI: https://ci.nodejs.org/job/node-test-commit/10055/

Member

addaleax commented May 21, 2017

@matthewloring I went ahead and pushed fixes for some of mine and Trevor’s review comments to this PR, feel free to remove anything that you disagree with.

Edit: Just so you know, I’m especially unsure about the last commit. I think it makes sense, but I’m not 100 % sure.

@trevnorris @AndreasMadsen Please take a look, and it would be great if you could pinpoint what you think should definitely be changed before this is ready to land.

CI: https://ci.nodejs.org/job/node-test-commit/10055/

@AndreasMadsen

LGTM. But @trevnorris should approve before merging.

Left a few comments, mostly just nits and questions.

Show outdated Hide outdated src/async-wrap.cc
Show outdated Hide outdated src/async-wrap.cc
}
process.on('exit', onexit);
function onexit() {

This comment has been minimized.

@AndreasMadsen

AndreasMadsen May 21, 2017

Member

We don't check the destroy hook. I'm okay with skipping it because it involves the gc and that can be a bit fiddly. But a comment should be added explaining why it isn't checked.

@AndreasMadsen

AndreasMadsen May 21, 2017

Member

We don't check the destroy hook. I'm okay with skipping it because it involves the gc and that can be a bit fiddly. But a comment should be added explaining why it isn't checked.

This comment has been minimized.

@matthewloring

matthewloring May 23, 2017

Contributor

Yup, done!

@matthewloring

matthewloring May 23, 2017

Contributor

Yup, done!

@addaleax addaleax referenced this pull request May 21, 2017

Closed

async_hooks: implement C++ embedder API #13142

4 of 4 tasks complete

@matthewloring matthewloring added this to the 8.0.0 milestone May 23, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 23, 2017

Member

@matthewloring The nice thing about e1c11e7 should be that the performance impact would only apply if one is using async hooks, so it doesn’t affect regular promise users.

Member

addaleax commented May 23, 2017

@matthewloring The nice thing about e1c11e7 should be that the performance impact would only apply if one is using async hooks, so it doesn’t affect regular promise users.

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring May 23, 2017

Contributor

e1c11e7 makes things faster when async hooks is disabled. #13175 makes things faster when they are enabled. I think we want both in the final product.

Contributor

matthewloring commented May 23, 2017

e1c11e7 makes things faster when async hooks is disabled. #13175 makes things faster when they are enabled. I think we want both in the final product.

@addaleax

Didn’t go through the test in detail but generally LGTM

Show outdated Hide outdated src/async-wrap.cc
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 23, 2017

Member

I think we want both in the final product.

Definitely, yes! I’m just eyeing a bit on the 8.0.0 release date, that’s all.

Member

addaleax commented May 23, 2017

I think we want both in the final product.

Definitely, yes! I’m just eyeing a bit on the 8.0.0 release date, that’s all.

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring May 23, 2017

Contributor

If we modify the shape of promises in this PR and then revert that after the 8.0 release will that be considered breaking? I would be ok working the promise internal field changes in afterwards if we can be pretty sure it will land.

Contributor

matthewloring commented May 23, 2017

If we modify the shape of promises in this PR and then revert that after the 8.0 release will that be considered breaking? I would be ok working the promise internal field changes in afterwards if we can be pretty sure it will land.

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring May 23, 2017

Contributor

In either case, I'll look into addressing the rest of Trevor's comments this afternoon.

Contributor

matthewloring commented May 23, 2017

In either case, I'll look into addressing the rest of Trevor's comments this afternoon.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 23, 2017

Member

If we modify the shape of promises in this PR and then revert that after the 8.0 release will that be considered breaking?

I don’t think so; I’d say it’s obvious those fields are internal, and we do consider async_hooks an experimental feature so I think we have the liberty to make that change.

I would be ok working the promise internal field changes in afterwards if we can be pretty sure it will land.

You can also rebase this PR on #13175 now, if you think that makes the most sense :) We’ll just consider this blocked until #13175 lands then.

Member

addaleax commented May 23, 2017

If we modify the shape of promises in this PR and then revert that after the 8.0 release will that be considered breaking?

I don’t think so; I’d say it’s obvious those fields are internal, and we do consider async_hooks an experimental feature so I think we have the liberty to make that change.

I would be ok working the promise internal field changes in afterwards if we can be pretty sure it will land.

You can also rebase this PR on #13175 now, if you think that makes the most sense :) We’ll just consider this blocked until #13175 lands then.

@addaleax addaleax referenced this pull request May 23, 2017

Closed

async_hooks: only set up hooks if used #13177

2 of 2 tasks complete
@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring May 24, 2017

Contributor

@trevnorris I think your comments should be addressed. Could you take another look.

Contributor

matthewloring commented May 24, 2017

@trevnorris I think your comments should be addressed. Could you take another look.

@AndreasMadsen

LGTM

As far as I can tell, this has been addressed.

@addaleax

This comment has been minimized.

Show comment
Hide comment
Member

addaleax commented May 24, 2017

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring
Contributor

matthewloring commented May 25, 2017

@matthewloring

This comment has been minimized.

Show comment
Hide comment
@matthewloring

matthewloring May 25, 2017

Contributor

I'll plan to land this tomorrow if there are no objections.

Contributor

matthewloring commented May 25, 2017

I'll plan to land this tomorrow if there are no objections.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 25, 2017

Member

Landed in 920278b, thanks a lot for all the work here!

Member

addaleax commented May 25, 2017

Landed in 920278b, thanks a lot for all the work here!

@addaleax addaleax closed this May 25, 2017

addaleax added a commit that referenced this pull request May 25, 2017

async_wrap,src: promise hook integration
This change provides unified tracking of asynchronous promise lifecycles
for both domains and async hooks.

PR-URL: #13000
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 May 25, 2017

Member

I still think if this returns the Promise as the resource that is probably the wrong behavior due to what had been discussed?

Member

Fishrock123 commented May 25, 2017

I still think if this returns the Promise as the resource that is probably the wrong behavior due to what had been discussed?

jasnell added a commit that referenced this pull request May 25, 2017

async_wrap,src: promise hook integration
This change provides unified tracking of asynchronous promise lifecycles
for both domains and async hooks.

PR-URL: #13000
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@AndreasMadsen AndreasMadsen referenced this pull request May 26, 2017

Closed

AsyncWrap #9

jasnell added a commit that referenced this pull request May 28, 2017

async_wrap,src: promise hook integration
This change provides unified tracking of asynchronous promise lifecycles
for both domains and async hooks.

PR-URL: #13000
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

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