Skip to content

async_wrap,src: promise hook integration #13000

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.

Sign up for GitHub

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

Closed
matthewloring wants to merge 16 commits into from
Closed

async_wrap,src: promise hook integration #13000

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ef35de1
async_wrap,src: promise hook integration
Apr 19, 2017
563c1b0
Reintroduce promise hook sharing
May 15, 2017
4aaadc0
Cleanup
May 15, 2017
70f9675
Clean up comments
May 23, 2017
0afbcfc
remove unused variables
addaleax May 21, 2017
833ad39
use .As<>() for casting
addaleax May 21, 2017
5717d75
align arguments (trevnorris review)
addaleax May 21, 2017
34ee31f
add CHECK_NE() (trevnorris review)
addaleax May 21, 2017
e0c194b
re-untangle domain/async_hooks promise support
addaleax May 21, 2017
e1c11e7
async_hooks: only set up hooks if used
addaleax May 21, 2017
d47e97a
Clarify use of additional object to track promise lifespan.
May 23, 2017
012171a
Clarifying comment about destroy hooks in test-promise.js
May 23, 2017
4e38bda
Remove redundant domain tracking
May 23, 2017
13313bb
Fix lint errors
May 24, 2017
9b9c310
Performance improvements
May 24, 2017
81ff7f6
Appease linter
May 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix lint errors
  • Loading branch information
Matt Loring committed May 24, 2017
commit 13313bbe3a4e6bd26bec760b66ce510e37557009
6 changes: 3 additions & 3 deletions src/async-wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,9 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
env->promise_wrap(),
v8::External::New(env->isolate(), wrap),
v8::PropertyAttribute::DontEnum).FromJust();
// The async tag will be destroyed at the same time as the promise as the only
// reference to it is held by the promise. This allows the promise wrap instance
// to be notified when the promise is destroyed.
// The async tag will be destroyed at the same time as the promise as the
// only reference to it is held by the promise. This allows the promise
// wrap instance to be notified when the promise is destroyed.
promise->DefineOwnProperty(context,
env->promise_async_tag(),
Copy link
Copy Markdown
Member

@AndreasMadsen AndreasMadsen May 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is promise_async_tag() used for, preventing garbage collection? If so I think a comment should be added.

Copy link
Copy Markdown
Author

@matthewloring matthewloring May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reference to it is held by the promise. We use its lifespan to approximate when the promise was garbage collected. I've added a comment describing this.

obj, v8::PropertyAttribute::DontEnum).FromJust();
Copy link
Copy Markdown
Contributor

@trevnorris trevnorris May 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want the user to be able to alter these? In src/async-wrap.cc I used

  v8::PropertyAttribute ReadOnlyDontDelete =
      static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete);

Though I probably should have added DontEnum to that list.

Copy link
Copy Markdown
Author

@matthewloring matthewloring May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property will be going away as soon as we move to an internal property on promises (blocked on #13175). I can modify this but I intend to get the promise internal property refactoring in before 8.0.

Copy link
Copy Markdown
Author

@matthewloring matthewloring May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this for now.

Expand Down
5 changes: 3 additions & 2 deletions test/parallel/test-async-wrap-getasyncid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,9 @@ function testInitialized(req, ctor_name) {


{
// We don't want to expose getAsyncId for promises but we need to construct one
// so that the cooresponding provider type is removed from the providers list.
// We don't want to expose getAsyncId for promises but we need to construct
// one so that the cooresponding provider type is removed from the
// providers list.
new Promise((res) => res(5));
Copy link
Copy Markdown
Member

@AndreasMadsen AndreasMadsen May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to expose the PromiseWrap on the new Promise object. PromiseWrap just has to be the resource object in the init hook, and the PromiseWrap object has to point to the new Promise object.

We don't expose *Wrap objects to userland in any other way than through the async_hooks resource object.

Copy link
Copy Markdown
Author

@matthewloring matthewloring May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does this mean that we do not need to expose a "getAsyncId" operation for promises (adding it will have performance implications so it would be great if it isn't needed)?

Copy link
Copy Markdown
Member

@AndreasMadsen AndreasMadsen May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I don't see any reason for getAsyncId to be exposed on promises.

Copy link
Copy Markdown
Author

@matthewloring matthewloring May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment to reflect this.

}

Expand Down