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

docs: documenting async hooks features #13287

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@AndreasMadsen
Member

AndreasMadsen commented May 29, 2017

Checklist
Affected core subsystem(s)
  • async hooks
  • docs

This is a continuation on #12953 it should "fix" all the suggestions.

It took me 3 hours fix all the issues, so I would like if nits are done by you in a followup PR I simply don't have this much time. Also please be very specific with the suggestions, as in replace "this" with "this".

thlorenz and others added some commits Nov 28, 2016

@addaleax

LGTM modulo typos (if it helps I can fix them myself and push to this PR)

Show outdated Hide outdated doc/api/async_hooks.md
If any `AsyncHook` callbacks throw, the application will print the stack trace
and exit. The exit path does follow that of an uncaught exception but
all `uncaughtException` listeners are removed, thus forceing the process to

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

typo: forceingforcing

@addaleax

addaleax May 29, 2017

Member

typo: forceingforcing

Show outdated Hide outdated doc/api/async_hooks.md
If any `AsyncHook` callbacks throw, the application will print the stack trace
and exit. The exit path does follow that of an uncaught exception but
all `uncaughtException` listeners are removed, thus forceing the process to
exit. The `'exit'` callbacks will still call unless the application is run with

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

callbe called

@addaleax

addaleax May 29, 2017

Member

callbe called

Show outdated Hide outdated doc/api/async_hooks.md
##### Printing in AsyncHooks callbacks
Because printing to the console is an asynchronous operations `console.log()`

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

typo: operationsoperation

@addaleax

addaleax May 29, 2017

Member

typo: operationsoperation

Show outdated Hide outdated doc/api/async_hooks.md
##### Printing in AsyncHooks callbacks
Because printing to the console is an asynchronous operations `console.log()`
will cause the AsyncHooks callbacks to write. Using `console.log()` or similar

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

writebe called

@addaleax

addaleax May 29, 2017

Member

writebe called

Show outdated Hide outdated doc/api/async_hooks.md
The `type` is a string that represents the type of resource that caused
`init()` to call. Generally it will be the name of the resource's constructor.
The resource types provided by the build in Node.js modules are:

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

typo: build inbuilt-in

@addaleax

addaleax May 29, 2017

Member

typo: build inbuilt-in

Show outdated Hide outdated doc/api/async_hooks.md
the user's callback is placed in a `process.nextTick()`.
The graph only shows **when** a resource was created. Not **why**. So to track
the **why** use `triggerId`.

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

typos: created. Not **why**. So tocreated, not **why**, so to

@addaleax

addaleax May 29, 2017

Member

typos: created. Not **why**. So tocreated, not **why**, so to

Show outdated Hide outdated doc/api/async_hooks.md
For example:
```js
const server = net.createServer(conn => {

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

I think we run the linter on docs now, so this might need to be (conn) instead of conn

@addaleax

addaleax May 29, 2017

Member

I think we run the linter on docs now, so this might need to be (conn) instead of conn

Show outdated Hide outdated doc/api/async_hooks.md
* Returns {undefined}
Call all `before()` callbacks and let them know a new asynchronous execution
context is being entered. If nested calls to `emitBefore()` are made the stack

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

comma after made

@addaleax

addaleax May 29, 2017

Member

comma after made

Show outdated Hide outdated doc/api/async_hooks.md
* Returns {undefined}
Call all `after()` callbacks. If nested calls to `emitBefore()` were made then

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

comma after made

@addaleax

addaleax May 29, 2017

Member

comma after made

Show outdated Hide outdated doc/api/async_hooks.md
make sure the stack is unwound properly. Otherwise an error will be thrown.
If the user's callback throws an exception then `emitAfter()` will
automatically be called for all `id`'s on the stack if the error is handled by

This comment has been minimized.

@addaleax

addaleax May 29, 2017

Member

remove the apostrophe (`id`'s`id`s)

@addaleax

addaleax May 29, 2017

Member

remove the apostrophe (`id`'s`id`s)

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 29, 2017

Member

LGTM modulo typos (if it helps I can fix them myself and push to this PR)

that would be awesome as I will go to bed soon. For minor things, I find this much more helpful.

Member

AndreasMadsen commented May 29, 2017

LGTM modulo typos (if it helps I can fix them myself and push to this PR)

that would be awesome as I will go to bed soon. For minor things, I find this much more helpful.

addaleax and others added some commits May 29, 2017

An asynchronous resource represents an object with an associated callback.
This callback may be called multiple times, for example, the `connection` event
in `net.createServer`, or just a single time like in `fs.open`. A resource

This comment has been minimized.

@mikesherov

mikesherov May 29, 2017

A word is missing somewhere in this sentence: a resource can also be closed the callback is called

@mikesherov

mikesherov May 29, 2017

A word is missing somewhere in this sentence: a resource can also be closed the callback is called

This comment has been minimized.

@AndreasMadsen

AndreasMadsen May 30, 2017

Member

Thanks.

a resource can also be closed the before callback is called

@AndreasMadsen

AndreasMadsen May 30, 2017

Member

Thanks.

a resource can also be closed the before callback is called

@AndreasMadsen AndreasMadsen referenced this pull request May 30, 2017

Closed

[async_hooks] tracking issue #29

23 of 27 tasks complete

@AndreasMadsen AndreasMadsen referenced this pull request May 30, 2017

Closed

docs: documenting async hooks features #12953

1 of 2 tasks complete
@trevnorris

Looking good. My bad on not seeing most of these before now.

Show outdated Hide outdated doc/api/async_hooks.md
An asynchronous resource represents an object with an associated callback.
This callback may be called multiple times, for example, the `connection` event
in `net.createServer`, or just a single time like in `fs.open`. A resource
can also be closed before the callback is called. AsyncHooks does not

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

The class name is actually AsyncHook. Reason the module is called async_hooks (plural) was to match events and timers (both plural but their class names aren't).

@trevnorris

trevnorris May 30, 2017

Contributor

The class name is actually AsyncHook. Reason the module is called async_hooks (plural) was to match events and timers (both plural but their class names aren't).

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

A resource can also be closed before the callback is called.

It looks like we're abstracting the difference between a request and an handle? Personally that would make me sad. Spent a lot of time precisely describing the difference between the two. :P

@trevnorris

trevnorris May 30, 2017

Contributor

A resource can also be closed before the callback is called.

It looks like we're abstracting the difference between a request and an handle? Personally that would make me sad. Spent a lot of time precisely describing the difference between the two. :P

This comment has been minimized.

@AndreasMadsen

AndreasMadsen May 31, 2017

Member

There are zero references to request and handle in the of the documentation (except for the previous terminology section). If request and handle are to be included, it should be described exactly how those concepts are visible in async_hooks.

see: #12953 (comment)

@AndreasMadsen

AndreasMadsen May 31, 2017

Member

There are zero references to request and handle in the of the documentation (except for the previous terminology section). If request and handle are to be included, it should be described exactly how those concepts are visible in async_hooks.

see: #12953 (comment)

Show outdated Hide outdated doc/api/async_hooks.md
// destroy() is called when an AsyncWrap instance is destroyed.
function destroy(id) { }
```

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

Should we just drop this whole section and replace it with common patterns and questions? e.g. "Handling events only once" in Events.

There are some corrections this section needs, but not going to comment b/c it may possibly be removed.

@trevnorris

trevnorris May 30, 2017

Contributor

Should we just drop this whole section and replace it with common patterns and questions? e.g. "Handling events only once" in Events.

There are some corrections this section needs, but not going to comment b/c it may possibly be removed.

This comment has been minimized.

@AndreasMadsen

AndreasMadsen Jun 1, 2017

Member

We definitely need a common patterns section, but let's not do it in this PR. When we write that I'm fine with removing this.

@AndreasMadsen

AndreasMadsen Jun 1, 2017

Member

We definitely need a common patterns section, but let's not do it in this PR. When we write that I'm fine with removing this.

Show outdated Hide outdated doc/api/async_hooks.md
Registers functions to be called for different lifetime events of each async
operation.
The callbacks `init()`/`before()`/`after()`/`destroy()` are registered via an

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

why the early return after "operation."? Looks like this should be a single paragraph.

@trevnorris

trevnorris May 30, 2017

Contributor

why the early return after "operation."? Looks like this should be a single paragraph.

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

I'd either simplify this to:

The callbacks init()/before()/after()/destroy() are called for the respective asynchronous event during a resource's lifetime.

Or take the time to explain what "registered" means:

The callbacks init()/before()/after()/destroy() are registered to a global list of hooks that are called for each respective asynchronous event during a resource's lifetime.

@trevnorris

trevnorris May 30, 2017

Contributor

I'd either simplify this to:

The callbacks init()/before()/after()/destroy() are called for the respective asynchronous event during a resource's lifetime.

Or take the time to explain what "registered" means:

The callbacks init()/before()/after()/destroy() are registered to a global list of hooks that are called for each respective asynchronous event during a resource's lifetime.

Show outdated Hide outdated doc/api/async_hooks.md
All callbacks are optional. So, for example, if only resource cleanup needs to
be tracked then only the `destroy()` callback needs to be passed. The
specifics of all functions that can be passed to `callbacks` is in the section
[`Hook Callbacks`][].

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

Possibly add that enabling an empty AsyncHook instance is a noop. Or add this in the enable() API section.

@trevnorris

trevnorris May 30, 2017

Contributor

Possibly add that enabling an empty AsyncHook instance is a noop. Or add this in the enable() API section.

track of what caused the asynchronous operation using the information
provided by AsyncHooks itself. The logging should then be skipped when
it was the logging itself that caused AsyncHooks callback to call. By
doing this the otherwise infinite recursion is broken.

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

I presented the idea elsewhere, but what about just disabling the ability to call a hook while other hooks are running? That would solve this easily (and I think it's reasonable that we don't need to give async statistics while the async hooks are running).

@trevnorris

trevnorris May 30, 2017

Contributor

I presented the idea elsewhere, but what about just disabling the ability to call a hook while other hooks are running? That would solve this easily (and I think it's reasonable that we don't need to give async statistics while the async hooks are running).

This comment has been minimized.

@AndreasMadsen

AndreasMadsen May 31, 2017

Member

please respond to #12953 (comment)

/cc @jasnell

@AndreasMadsen
Show outdated Hide outdated doc/api/async_hooks.md
The `init()` hook will trigger when an `AsyncResource` is instantiated.
It is important that before/after calls are unwined

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

We should make the document consistent between before()/after() or before/after or before/after. No personal preference, but is more aesthetically pleasing.

@trevnorris

trevnorris May 30, 2017

Contributor

We should make the document consistent between before()/after() or before/after or before/after. No personal preference, but is more aesthetically pleasing.

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

Instead:

It is important that before/after calls are unwound in the same order [...]

@trevnorris

trevnorris May 30, 2017

Contributor

Instead:

It is important that before/after calls are unwound in the same order [...]

// Return the trigger ID for the AsyncResource instance.
asyncResource.triggerId();
```

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

If this is supposed to be an API overview then it should probably have the sub-heading that says that. But it may also want to be removed, like I discussed above or the other similar section.

@trevnorris

trevnorris May 30, 2017

Contributor

If this is supposed to be an API overview then it should probably have the sub-heading that says that. But it may also want to be removed, like I discussed above or the other similar section.

* arguments
* `type` {string} the type of ascyc event
* `triggerId` {number} the ID of the execution context that created this async
event

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

Add that the default is currentId()

@trevnorris

trevnorris May 30, 2017

Contributor

Add that the default is currentId()

This comment has been minimized.

@AndreasMadsen

AndreasMadsen May 31, 2017

Member

Again, we can't document details in the summary.

@AndreasMadsen

AndreasMadsen May 31, 2017

Member

Again, we can't document details in the summary.

Show outdated Hide outdated doc/api/async_hooks.md
never called, causing a memory leak in the application. Of course if
the resource doesn't depend on GC then this isn't an issue.
#### `async_hooks.currentId()`

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

I think this should either be changed to asyncId() or currentAsyncId() (to follow the change for the function argument names). If the latter then triggerId() should probably be changed to currentTriggerId(). I'm down to hear alternatives, but would like to see it be more explicit and uniform. (sorry, my bad)

@trevnorris

trevnorris May 30, 2017

Contributor

I think this should either be changed to asyncId() or currentAsyncId() (to follow the change for the function argument names). If the latter then triggerId() should probably be changed to currentTriggerId(). I'm down to hear alternatives, but would like to see it be more explicit and uniform. (sorry, my bad)

This comment has been minimized.

@AndreasMadsen

AndreasMadsen May 31, 2017

Member

This would have been a really nice comment to make before node.js 8 was released.

/cc @addaleax

@AndreasMadsen

AndreasMadsen May 31, 2017

Member

This would have been a really nice comment to make before node.js 8 was released.

/cc @addaleax

This comment has been minimized.

@addaleax

addaleax May 31, 2017

Member

Yeah, it would be nice to have done that before, but I don’t think it needs to be a big deal.

currentAsyncId() and currentTriggerId() sound fine to me. We can always make the old names (non-enumerable?) aliases for those, so we don’t break any existing code.

@addaleax

addaleax May 31, 2017

Member

Yeah, it would be nice to have done that before, but I don’t think it needs to be a big deal.

currentAsyncId() and currentTriggerId() sound fine to me. We can always make the old names (non-enumerable?) aliases for those, so we don’t break any existing code.

This comment has been minimized.

@AndreasMadsen

AndreasMadsen Jun 1, 2017

Member

I'm okay with renaming it, but I think it would be much better to do that in another PR.

@AndreasMadsen

AndreasMadsen Jun 1, 2017

Member

I'm okay with renaming it, but I think it would be much better to do that in another PR.

Show outdated Hide outdated doc/api/async_hooks.md
```js
class DBQuery extends AsyncResource {
constructor(db) {
super();

This comment has been minimized.

@trevnorris

trevnorris May 30, 2017

Contributor

May want to do super('DBQuery'). Since this would throw. from source:

    if (typeof type !== 'string' || type.length <= 0)
      throw new TypeError('type must be a string with length > 0');
@trevnorris

trevnorris May 30, 2017

Contributor

May want to do super('DBQuery'). Since this would throw. from source:

    if (typeof type !== 'string' || type.length <= 0)
      throw new TypeError('type must be a string with length > 0');
@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 31, 2017

Member

@trevnorris thanks for the review. I'm not going to fix any of this in the next 24 hours. I have addressed the comments I don't understand.

Member

AndreasMadsen commented May 31, 2017

@trevnorris thanks for the review. I'm not going to fix any of this in the next 24 hours. I have addressed the comments I don't understand.

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris May 31, 2017

Contributor

@AndreasMadsen No worries. I'll get to those you did leave comments on.

Contributor

trevnorris commented May 31, 2017

@AndreasMadsen No worries. I'll get to those you did leave comments on.

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen Jun 1, 2017

Member

@trevnorris please take a look.

  • I don't see a need for the handle and request terminology, I've described some different cases/examples in the before().
  • I didn't add the common patterns section, I think we can do that in a follow-up PR. When that is done I'm fine with removing some of the API overviews.
  • I didn't rename async_hooks.currentId(). I just want this to be a documentation PR, we can rename it in a follow-up PR.
Member

AndreasMadsen commented Jun 1, 2017

@trevnorris please take a look.

  • I don't see a need for the handle and request terminology, I've described some different cases/examples in the before().
  • I didn't add the common patterns section, I think we can do that in a follow-up PR. When that is done I'm fine with removing some of the API overviews.
  • I didn't rename async_hooks.currentId(). I just want this to be a documentation PR, we can rename it in a follow-up PR.
@trevnorris

Much thanks for getting this ready.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Jun 1, 2017

Member

Landed in 126170a 🎉

(fixed up 2 linter complaints while landing.)

Member

addaleax commented Jun 1, 2017

Landed in 126170a 🎉

(fixed up 2 linter complaints while landing.)

@addaleax addaleax closed this Jun 1, 2017

@AndreasMadsen AndreasMadsen deleted the AndreasMadsen:async-hooks-docs branch Jun 1, 2017

@AndreasMadsen AndreasMadsen referenced this pull request Jun 4, 2017

Closed

async_hooks: use resource objects for Promises #13452

4 of 4 tasks complete

jasnell added a commit that referenced this pull request Jun 5, 2017

doc: async-hooks documentation
Author: Thorsten Lorenz <thlorenz@gmx.de>
Author: Andreas Madsen <amwebdk@gmail.com>
PR-URL: #13287
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@jasnell jasnell referenced this pull request Jun 5, 2017

Closed

8.1.0 Proposal #13483

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jul 17, 2017

Member

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

Member

MylesBorins commented Jul 17, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

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