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

docs: async-hooks documentation #2

Conversation

thlorenz
Copy link
Owner

@thlorenz thlorenz commented Nov 28, 2016

WIP PR to get Feedback.

@rvagg @Fishrock123 @trevnorris

Copy link

@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.

Docs will need to be word wrapped before going into core. I'll let you decide when to take care of that.

all optional. Returns an `AsyncHook`


* `init` {function}

Choose a reason for hiding this comment

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

nit: the value inside of the {} is usually capitalized in core docs at least.

Copy link
Owner Author

Choose a reason for hiding this comment

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

cool will fix



function init(uid, type, triggerId, handle) {
const activity = { uid, type, parentUid }

Choose a reason for hiding this comment

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

parentUid -> triggerId

process.on('exit', onexit)
function onexit() {
// Stop listening on the above hooks
asyncHook.disable()

Choose a reason for hiding this comment

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

thought: should we automatically stop listening in the 'exit' event?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'd opt for not doing any magic here. It doesn't matter much anyways since the process is exiting ...

* `uid` {number} a unique id for the async operation


### AsyncHook.enable

Choose a reason for hiding this comment

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

This should either be asyncHook.enable() or AsyncHook#enable(). Like this it looks like a static member (i.e. not one on the prototype).

Copy link
Owner Author

Choose a reason for hiding this comment

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

which one of these two is more common given this is a module method? cc @Fishrock123

### async_hooks.currentId()


Returns the uid of the current context.

Choose a reason for hiding this comment

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

nit: "current execution context". remove a hair more ambiguity.

embedder API.


## Embedder API

Choose a reason for hiding this comment

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

The preferred embedder API is to use AsyncEvent. The async_hooks.emitInit(), etc., were made for internals, but I could find reasonable need for them in user code. But want to make clear they should attempt to use AsyncEvent first, if possible.

Copy link
Owner Author

Choose a reason for hiding this comment

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

OK will dig into that a bit more AsyncEvent and update the docs as far as I'll understand things at that point ...

@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from cb4c5f2 to 8745613 Compare January 4, 2017 15:16
@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from f068849 to 620fb61 Compare January 5, 2017 14:27


The `async-hooks` module provides an API to register callbacks tracking the lifetime of asynchronous handles created
inside a Node.js application.

Choose a reason for hiding this comment

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

Needs a terminology section: https://github.com/nodejs/node-eps/blob/e35d4813fdbbd99a751296e24361dba0d0dd9e10/XXX-asyncwrap-api.md#terminology

The top-level description should probably not include the word "handle".

Copy link

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

So, al the Node.js docs follow some order:

All sections and content within them must be alphabetical, the exception being if you have API and subheadings in the same spot, then the API references must be alphabetical but the sub-heading should usually go last.

Also, we tend to put examples after API references, except in the case of a top-level example.

@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from 8745613 to 13c8190 Compare January 17, 2017 16:01
@thlorenz
Copy link
Owner Author

@Fishrock123 @trevnorris, ready for another review. Fixed a bunch of nits and integrated big parts from the EPS

Most now comes from this EPS, except I reordered things a bit to clearly separate the different parts of the API.
I also removed some parts that described implementation details not useful to the users.

I also removed the original example I had in there, as now lots of examples are already included in numerous places.

LMK what you think at this point.

@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch 4 times, most recently from 97194bd to 7dae377 Compare January 25, 2017 21:22
@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from 175108c to df8e8e5 Compare January 26, 2017 16:28
Copy link

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

I think it would also be good to have a section explaining that you must sue process._rawDebug() to log within a look so as to prevent an infinite loop.



> Stability: 2 - Stable

Choose a reason for hiding this comment

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

extra spaces?

Copy link
Owner Author

Choose a reason for hiding this comment

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

could you clarify? Missing spaces, need to add them and where?

Requests are short lived data structures created to accomplish one task. The
callback for a request should always and only ever fire one time. Which is when
the assigned task has either completed or encountered an error. Requests are
used by handles to perform these tasks. Such as accepting a new connection or

Choose a reason for hiding this comment

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

tasks, such as -- the last sentence probably shouldn't be it's own.

Copy link
Owner Author

Choose a reason for hiding this comment

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

OK will fix.

const async_hooks = require('async_hooks');

// Return the id of the current execution context. Useful for tracking state
// and retrieving the handle of the current trigger without needing to use an

Choose a reason for hiding this comment

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

You mean retrieving the handle if the user has stored it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Should probably be id instead of handle .. I agree it's confusing.
This came from the EP (slightly adapted) so @trevnorris should know.

Choose a reason for hiding this comment

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

... retrieving the handle of the current trigger

That comment dates back to when I had implemented a map to keep track of all resources, and there was async_hooks.getResourceById(). Which would return the resource from the map, but it ended up being too costly. So it was removed.

// current execution scope to fire.
const tid = aysnc_hooks.triggerId();

// Create a new instance of AsyncHook. All of these callbacks are optional.

Choose a reason for hiding this comment

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

a new AsyncHook instance.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hairsplitting, but I've got no strong opinion, so will fix.


// Disable listening for new asynchronous events. Though this will not prevent
// callbacks from firing on asynchronous chains that have already run within
// the scope of an enabled AsyncHook instance.

Choose a reason for hiding this comment

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

It will actually prevent those

Copy link
Owner Author

Choose a reason for hiding this comment

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

Need clarification from @trevnorris here.

Choose a reason for hiding this comment

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

I updated the EP to simply say:

// Disable listening for all new asynchronous events.

}):
```

It is important to note that the id returned has to do with execution timing.

Choose a reason for hiding this comment

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

returned from `currentId()`

Copy link
Owner Author

Choose a reason for hiding this comment

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

Will fix.


The class `AsyncEvent` was designed to be extended from for embedder's async
resources. Using this users can easily trigger the lifetime events of their
own resources.

Choose a reason for hiding this comment

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

Perhaps we want to put in a thing about that I/O pools need to use this?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Need more details on that or maybe you or @trevnorris can suggest the addition here?

Choose a reason for hiding this comment

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

@Fishrock123 can you clarify what you mean?


// Call after() hooks. It is important that before/after calls are unwound
// in the same order they are called. Otherwise an unrecoverable exception
// will be made.

Choose a reason for hiding this comment

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

Perhaps this comment should go under the actual api header with an example?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, makes sense. Will fix.

#### `AsyncEvent(type[, triggerId])`

* arguments
* type {String} the type of ascync event

Choose a reason for hiding this comment

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

async

Copy link
Owner Author

Choose a reason for hiding this comment

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

Will fix.

Choose a reason for hiding this comment

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

The argument names are supposed to be surrounded in backticks. e.g. `type`. Please change throughout the document.

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
a domain or `'uncaughtException'` handler. So there is no need to guard against
this.

Choose a reason for hiding this comment

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

Perhaps we should say it should not be called or an example given?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Not fully sure what you mean. What should not be called?

@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from df8e8e5 to b3aed32 Compare January 26, 2017 19:33
@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 33c2eb5 to 97a27f1 Compare January 27, 2017 15:29
@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from b3aed32 to dc4833d Compare January 27, 2017 15:30
Copy link

@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.

Thanks for the work. I like the added example of using the AsyncEvent API.

# Async Hooks


> Stability: 2 - Stable

Choose a reason for hiding this comment

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

I believe the initial release will be labeled 1 - Experimental so that we have a major version to make any necessary tweaks.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed

lifetime of asynchronous resources created inside a Node.js application.

At its simplest form it can track metadata about each resource and log it when
the program exits.

Choose a reason for hiding this comment

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

This doesn't make much sense to me. The absolute simplest would be to log metadata about asynchronous resources when they occur.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Removed the paragraph

const async_hooks = require('async_hooks');

// Return the id of the current execution context. Useful for tracking state
// and retrieving the handle of the current trigger without needing to use an

Choose a reason for hiding this comment

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

... retrieving the handle of the current trigger

That comment dates back to when I had implemented a map to keep track of all resources, and there was async_hooks.getResourceById(). Which would return the resource from the map, but it ended up being too costly. So it was removed.


// Disable listening for new asynchronous events. Though this will not prevent
// callbacks from firing on asynchronous chains that have already run within
// the scope of an enabled AsyncHook instance.

Choose a reason for hiding this comment

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

I updated the EP to simply say:

// Disable listening for all new asynchronous events.


Registers functions to be called for different lifetime events of each async
operation.
The above callbacks registered via an `AsyncHooks` instance fire during specific

Choose a reason for hiding this comment

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

If that is removed I'd suggest explicitly mentioning all four callbacks. e.g.

The callbacks init()/before()/after()/destroy() are registered via...

async_hooks.createHook({
init: (id, type, triggerId) => {
const cId = async_hooks.currentId();
process._rawDebug(' '.repeat(ws) +

Choose a reason for hiding this comment

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

Did you add that console.*() should never be used from hook callbacks? May need to mention this.

SIGNALWRAP(7): trigger: 4 scope: 4
>>> 4
after: 4
after: 5

Choose a reason for hiding this comment

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

The script above, and this output, were both updated in the EP. Please reference.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updated the output .. didn't see any changes to the script.
Note that we're using class like function declarations (instead of arrow functions) as per request by @Fishrock123

**Note:** Some resources depend on GC for cleanup. So if a reference is made to
the `resource` object passed to `init()` it's possible that `destroy()` is
never called. Causing a memory leak in the application. Of course if you know
the resource doesn't depend on GC then this isn't an issue.

Choose a reason for hiding this comment

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

It's anything that uses MakeWeak() in C++. Can throw that list together if needed.


The class `AsyncEvent` was designed to be extended from for embedder's async
resources. Using this users can easily trigger the lifetime events of their
own resources.

Choose a reason for hiding this comment

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

@Fishrock123 can you clarify what you mean?

#### `AsyncEvent(type[, triggerId])`

* arguments
* type {String} the type of ascync event

Choose a reason for hiding this comment

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

The argument names are supposed to be surrounded in backticks. e.g. `type`. Please change throughout the document.

@thlorenz
Copy link
Owner Author

@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 97a27f1 to 79b11ce Compare February 1, 2017 15:15
@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from 17edc94 to e5193f7 Compare February 1, 2017 15:16
@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 79b11ce to 1ff2dfb Compare February 2, 2017 15:44
@thlorenz thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from e5193f7 to 4dac48a Compare February 2, 2017 15:50
@thlorenz
Copy link
Owner Author

thlorenz commented Feb 7, 2017

So I fixed the nits pointed out by Trevor, but still need some clarification from @Fishrock123 for some of the above.

TTYWRAP(6) -> Timeout(4) -> TIMERWRAP(5) -> TickObject(3) -> root(1)
```

The `TCPWRAP` isn't part of this graph; evne though it was the reason for

Choose a reason for hiding this comment

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

s/evne/even/

```js
const async_hooks = require('async_hooks');

// Return the id of the current execution context.

Choose a reason for hiding this comment

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

Probably worth defining 'execution context' somewhere.

hostname is actually synchronous, but to maintain a completely asynchronous API
the user's callback is placed in a `process.nextTick()`.

The graph only shows **when** a resource was created. Not **why**. So to track

Choose a reason for hiding this comment

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

Not why. So to track the why use triggerId.

This is a bit awkward

event
* Returns {AsyncEvent} A reference to `asyncHook`.

Example usage:

Choose a reason for hiding this comment

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

This seems less like an embedder's use but an APM/wrapper's use. I'd think that the embedder would put the emitBefore and emitAfterinsidefunction this.db.getnot thegetInfo` wrapper?

@thlorenz
Copy link
Owner Author

@brycebaril let's continue discussion here: nodejs#12953

const async_hooks = require('async_hooks');

// Return new unique id for a constructing handle.
const id = async_hooks.newUid();
Copy link

Choose a reason for hiding this comment

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

Should this be newId()?

@thlorenz thlorenz closed this Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants