Skip to content
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_hooks: docs and api ergonomics tweaks #15103

Closed
wants to merge 5 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 30, 2017

Some doc improvements and a AsyncResource constructor ergonomics tweak...

Previously, if type was passed to the AsyncResource class as null or undefined,
an error would be thrown. With this, type defaults to new.target.name.

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_hooks

@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Aug 30, 2017
@jasnell jasnell requested review from trevnorris and addaleax August 30, 2017 22:17
@jasnell jasnell changed the title Async hooks tweaks async_hooks: docs and api ergonomics tweaks Aug 30, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Code LGTM.
Semi-Rubberstamp the docs.

@refack
Copy link
Contributor

refack commented Aug 30, 2017

/cc @nodejs/async_hooks

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

Left a few possible changes about the docs


In the case of Promises, the `resource` object will have `promise` property
that refers to the Promise that is being initialized, and a `parentId` property
that equals the `asyncId` of a parent Promise, if there is one, and
that set to the `asyncId` of a parent Promise, if there is one, and
Copy link
Contributor

Choose a reason for hiding this comment

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

that isn't needed anymore

considered public, but using the Embedder API users can provide and document
their own resource objects. Such as resource object could for example contain
the SQL query being executed.
useful information that can vary based on the value of `type`. For instance,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe swap out it for resource? was slightly unclear what it was referring to

*Note:* Some resources depend on garbage collection for cleanup, so if a
reference is made to the `resource` object passed to `init` it is possible that
`destroy` will never be called, causing a memory leak in the application. If
the resource does not depend on garbage collection then this will not be an
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a comma is needed after not depend on garbage collection

* `type` {string} the type of ascyc event
* `triggerAsyncId` {number} the ID of the execution context that created this async
event
* `type` {string} the type of ascyc event. If `type` is `null` or `undefined`,
Copy link
Contributor

Choose a reason for hiding this comment

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

ascyc --> async


Below is another example with additional information about the calls to
Following is an example with additional information about the calls to
Copy link
Contributor

Choose a reason for hiding this comment

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

Not completely sure about this one, but should this be The following? 😬

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

I'm not so sure about the type default.

// If type is null or undefined, set it by default
// to the new.target.name. This avoids an error when
// emitInitScript is called.
if (type == null)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't default then we should TypeError if the type isn't provided rather than the more obscure error that throws now.

@jasnell jasnell force-pushed the async_hooks_tweaks branch 2 times, most recently from 9c2f1d5 to def3c83 Compare September 1, 2017 18:19
@jasnell
Copy link
Member Author

jasnell commented Sep 1, 2017

@AndreasMadsen ... ok, I've reworked this to take the approach of throwing instead of defaulting. PTAL.

@refack ... please take another look also.

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member Author

jasnell commented Sep 5, 2017

@refack
Copy link
Contributor

refack commented Sep 5, 2017

We could do some behind the scenes juggling like if (new.target.name === 'AsyncResource') throw "AsyncResource is an abstract class";
And lazy register the name from the constructor in a #13610 like mechanism.

But that could wait for a future PR. Explicit type is a decent solution, considering it's the only discriminating property of the hooked events.

@BridgeAR
Copy link
Member

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 13, 2017
@jasnell
Copy link
Member Author

jasnell commented Sep 13, 2017

With the change to the error handling, this actually becomes a semver-major.

@addaleax and @trevnorris ... I'd appreciate a review on this.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 13, 2017

@jasnell async_hooks is still experimental. Therefore this should not be semver-major.

@jasnell
Copy link
Member Author

jasnell commented Sep 13, 2017

Yes, technically that's correct, but given that they've been around for a while now, I think it's prudent to be careful

Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

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

have a couple questions, but nothing that needs to be done.


*Note*: In some cases the resource object is reused for performance reasons,
it is thus not safe to use it as a key in a `WeakMap` or add properties to it.

###### asynchronous context example
###### Asynchronous context example
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure of any current style, but should headers capitalize all words? doesn't matter to me either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

heck if I know. I don't think we've had much of a consistent style here.

`console.log()` being called. This is because binding to a port without a
hostname is actually synchronous, but to maintain a completely asynchronous API
the user's callback is placed in a `process.nextTick()`.
hostname is a *synchronous* operation within Node.js, but to maintain a
Copy link
Contributor

Choose a reason for hiding this comment

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

is "within Node.js" necessary? possibly we're referring to something else, but i'm not sure what.

Copy link
Member Author

Choose a reason for hiding this comment

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

Likely not necessary.

appropriate callbacks are called. To accommodate this a JavaScript API is
provided.
Library developers that handle their own asychronous resources performing tasks
like I/O, connection pooling, or managing callback queues may use the AsyncWrap
Copy link
Contributor

Choose a reason for hiding this comment

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

should AsyncWrap be placed in backticks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep.

@jasnell
Copy link
Member Author

jasnell commented Sep 14, 2017

@nodejs/tsc ... this needs another tsc member to review (unless folks are ok with it not being semver-major)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM as semver-minor.
I prefer this to land in 8 than not have it in there.

What's the plan for moving async-hooks out of experimental?

@AndreasMadsen
Copy link
Member

What's the plan for moving async-hooks out of experimental?

@mcollina Let's have the discussion in #14717. I've made a comment on what I think is required #14717 (comment).

@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 15, 2017
jasnell added a commit that referenced this pull request Sep 15, 2017
Update docs and type checking for AsyncResource type

PR-URL: #15103
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Sep 15, 2017

Landed in d8a0364

@jasnell jasnell closed this Sep 15, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
Update docs and type checking for AsyncResource type

PR-URL: nodejs/node#15103
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Sep 20, 2017
Update docs and type checking for AsyncResource type

PR-URL: #15103
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
Update docs and type checking for AsyncResource type

PR-URL: nodejs/node#15103
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants