docs: async-hooks documentation #2

thlorenz · 2016-11-28T12:09:31Z

WIP PR to get Feedback.

trevnorris

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

trevnorris · 2016-12-01T23:32:29Z

doc/api/async_hooks.md

+all optional. Returns an `AsyncHook`
+
+
+* `init` {function}


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

cool will fix

trevnorris · 2016-12-01T23:33:12Z

doc/api/async_hooks.md

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


parentUid -> triggerId

trevnorris · 2016-12-01T23:33:57Z

doc/api/async_hooks.md

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


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

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

trevnorris · 2016-12-01T23:35:16Z

doc/api/async_hooks.md

+    * `uid` {number} a unique id for the async operation
+
+
+### AsyncHook.enable


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

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

trevnorris · 2016-12-01T23:36:00Z

doc/api/async_hooks.md

+### async_hooks.currentId()
+
+
+Returns the uid of the current context.


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

trevnorris · 2016-12-01T23:37:29Z

doc/api/async_hooks.md

+embedder API.
+
+
+## Embedder API


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.

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

Fishrock123 · 2017-01-05T15:03:13Z

doc/api/async_hooks.md

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


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

Fishrock123

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 · 2017-01-17T20:35:56Z

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

Fishrock123

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.

Fishrock123 · 2017-01-26T16:28:52Z

doc/api/async_hooks.md

+
+
+> Stability: 2 - Stable
+


extra spaces?

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

Fishrock123 · 2017-01-26T16:31:45Z

doc/api/async_hooks.md

+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


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

OK will fix.

Fishrock123 · 2017-01-26T16:44:12Z

doc/api/async_hooks.md

+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


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

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

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

Fishrock123 · 2017-01-26T16:44:58Z

doc/api/async_hooks.md

+// current execution scope to fire.
+const tid = aysnc_hooks.triggerId();
+
+// Create a new instance of AsyncHook. All of these callbacks are optional.


a new AsyncHook instance.

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

Fishrock123 · 2017-01-26T17:25:04Z

doc/api/async_hooks.md

+
+// 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.


It will actually prevent those

Need clarification from @trevnorris here.

I updated the EP to simply say:

// Disable listening for all new asynchronous events.

Fishrock123 · 2017-01-26T17:48:17Z

doc/api/async_hooks.md

+}):
+```
+
+It is important to note that the id returned has to do with execution timing.


returned from `currentId()`

Fishrock123 · 2017-01-26T17:49:22Z

doc/api/async_hooks.md

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


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

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

@Fishrock123 can you clarify what you mean?

Fishrock123 · 2017-01-26T17:50:13Z

doc/api/async_hooks.md

+
+// 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.


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

Yeah, makes sense. Will fix.

Fishrock123 · 2017-01-26T17:50:44Z

doc/api/async_hooks.md

+#### `AsyncEvent(type[, triggerId])`
+
+* arguments
+  * type {String} the type of ascync event


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

Fishrock123 · 2017-01-26T17:51:29Z

doc/api/async_hooks.md

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


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

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

trevnorris

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

trevnorris · 2017-01-27T23:31:48Z

doc/api/async_hooks.md

+# Async Hooks
+
+
+> Stability: 2 - Stable


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

trevnorris · 2017-01-27T23:33:13Z

doc/api/async_hooks.md

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


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

Removed the paragraph

trevnorris · 2017-01-27T23:35:56Z

doc/api/async_hooks.md

+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


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

trevnorris · 2017-01-27T23:36:49Z

doc/api/async_hooks.md

+
+// 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.


I updated the EP to simply say:

// Disable listening for all new asynchronous events.

trevnorris · 2017-01-27T23:38:47Z

doc/api/async_hooks.md

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


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

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

trevnorris · 2017-01-27T23:44:06Z

doc/api/async_hooks.md

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


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

trevnorris · 2017-01-27T23:45:08Z

doc/api/async_hooks.md

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


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

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

trevnorris · 2017-01-27T23:47:19Z

doc/api/async_hooks.md

+**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.


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

trevnorris · 2017-01-27T23:48:58Z

doc/api/async_hooks.md

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


@Fishrock123 can you clarify what you mean?

trevnorris · 2017-01-27T23:50:33Z

doc/api/async_hooks.md

+#### `AsyncEvent(type[, triggerId])`
+
+* arguments
+  * type {String} the type of ascync event


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

thlorenz · 2017-01-30T17:41:57Z

@Fishrock123 @trevnorris addressed the nits.

Still need input/clarification on the below

thlorenz · 2017-02-07T17:16:14Z

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

brycebaril · 2017-03-31T16:37:56Z

doc/api/async_hooks.md

+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


s/evne/even/

brycebaril · 2017-03-31T16:38:27Z

doc/api/async_hooks.md

+```js
+const async_hooks = require('async_hooks');
+
+// Return the id of the current execution context.


Probably worth defining 'execution context' somewhere.

brycebaril · 2017-03-31T16:41:09Z

doc/api/async_hooks.md

+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


Not why. So to track the why use triggerId.

This is a bit awkward

brycebaril · 2017-03-31T16:51:54Z

doc/api/async_hooks.md

+    event
+* Returns {AsyncEvent} A reference to `asyncHook`.
+
+Example usage:


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 · 2017-05-10T21:18:34Z

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

drewfish · 2017-06-02T13:37:58Z

doc/api/async_hooks.md

+const async_hooks = require('async_hooks');
+
+// Return new unique id for a constructing handle.
+const id = async_hooks.newUid();


Should this be newId()?

trevnorris reviewed Dec 1, 2016

View reviewed changes

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from cb4c5f2 to 8745613 Compare January 4, 2017 15:16

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from f068849 to 620fb61 Compare January 5, 2017 14:27

Fishrock123 suggested changes Jan 5, 2017

View reviewed changes

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

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 force-pushed the trevnorris-async-wrap-eps-impl+docs branch from 175108c to df8e8e5 Compare January 26, 2017 16:28

Fishrock123 reviewed Jan 26, 2017

View reviewed changes

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from df8e8e5 to b3aed32 Compare January 26, 2017 19:33

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 33c2eb5 to 97a27f1 Compare January 27, 2017 15:29

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from b3aed32 to dc4833d Compare January 27, 2017 15:30

trevnorris suggested changes Jan 27, 2017

View reviewed changes

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 97a27f1 to 79b11ce Compare February 1, 2017 15:15

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from 17edc94 to e5193f7 Compare February 1, 2017 15:16

thlorenz force-pushed the trevnorris-async-wrap-eps-impl branch from 79b11ce to 1ff2dfb Compare February 2, 2017 15:44

docs: async-hooks documentation

4dac48a

thlorenz force-pushed the trevnorris-async-wrap-eps-impl+docs branch from e5193f7 to 4dac48a Compare February 2, 2017 15:50

docs: addressing nits pointed out by Trevor

5d7a544

brycebaril reviewed Mar 31, 2017

View reviewed changes

thlorenz mentioned this pull request May 22, 2017

docs: documenting async hooks features nodejs/node#12953

Closed

2 tasks

drewfish reviewed Jun 2, 2017

View reviewed changes

thlorenz closed this Oct 27, 2017



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

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


		### AsyncHook.enable

		### async_hooks.currentId()


		Returns the uid of the current context.



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

docs: async-hooks documentation #2

docs: async-hooks documentation #2

Conversation

thlorenz commented Nov 28, 2016 • edited Loading

trevnorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

thlorenz commented Jan 17, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thlorenz commented Jan 30, 2017

thlorenz commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thlorenz commented May 10, 2017

Choose a reason for hiding this comment

thlorenz commented Nov 28, 2016 •

edited

Loading