async_wrap: add uid argument to all asyncWrap hooks

[AndreasMadsen]

By doing this, users can use a Map object for storing information instead of modifying the handle object.

@trevnorris mentioned this was an "API deficiency" https://github.com/nodejs/tracing-wg/pull/37/files#r48493114

Also, could we make uid the first argument in the init hook, such that it consistent for all hooks?

[AndreasMadsen]

Also, could we make uid the first argument in the init hook, such that it consistent for all hooks?

Added a commit b59518387ce3c151a52772bcd4c220b7400a98ab

[trevnorris]

CI: https://ci.nodejs.org/job/node-test-pull-request/1197/

LGTM. Great stuff. Thanks for taking care of this.

[AndreasMadsen]

Actually it is still necessary to mutate the handle object, because there is otherwise no way of relating the parent handle object to the corresponding user object. Should we replace the parent handle object with the parent uid?

[trevnorris]

@AndreasMadsen Can you expound on that?

[AndreasMadsen]

The long-stack-trace example explains it well:

const uidSymbol = Symbol('uid');
const stacks = new Map();
let activeHandleUid = -1;
function init (uid, provider, parent) {
  this[uidSymbol] = uid;

  const parentStack = stack.get(parent ? parent[uidSymbol] : activeHandleUid);
  const fullstack = combineStack(new Error(), parentStack);
  stacks.set(uid, fullstack);
}
function pre (uid) {
  activeHandleUid = uid;
}
function post (uid) {
  activeHandleUid = -1;
}
function destroy (uid) {
  stacks.delete(uid);
}

By providing the parent uid such that the init hook becomes init (uid, provider, parentUid) the parent[uidSymbol] hack isn't necessary. I've added a commit d2e2c65e0fd20c7c8813f31cf79dea3642533c3d that implements this.

[Qard]

Nice! 👍 for parentUid.

[trevnorris]

@AndreasMadsen That makes sense. Guess the initial reason I did this was so you could WeakMap the parent, to make sure it was impossible to both leak, and reference if the parent didn't last as long as the child (think that may be possible in at least one case, but will need to investigate further).

[AndreasMadsen]

Guess the initial reason I did this was so you could WeakMap the parent ...

Yeah, that makes sense. But with the introduction of the destroy hook uid is now the common key.

PS (long): For some odd reason WeekMap does not work very well for storing custom user data (it might be a v8 bug, but I haven't investigated it that much). See: AndreasMadsen/trace#17

The graphs tells the story:

Mutating the handle object

WeakMap using handle object

Map using uid and destructor event

[trevnorris]

@AndreasMadsen Don't get me wrong. I didn't say it was a good solution. Only that it was a solution. Passing the id to all callbacks is preferable.

[AndreasMadsen]

Can we land this?

PS: I had to rebase because of efd33a2

[jasnell]

@trevnorris ... ping. Does this still look good to you?

[trevnorris]

@AndreasMadsen Sorry for the delay.

It just occurred to me one reason I decided to pass the parent handle. Because if the user does something like:

const async_wrap = process.binding('async_wrap');
async_wrap.setupHooks();

require('net').createServer().listen();

async_wrap.enable();

Then there will be no way to reference the parent handle from the new connection simply by the uid.

Any thoughts on this?

I was considering if it were possible to Unwrap() the parent passed to the constructor and store that as a field. Then have some sort of parent() getter that returns it from native. But haven't had a chance to prototype this approach.

[trevnorris]

@AndreasMadsen Actually how about this. Move the parent object to the last argument for create and remove it from destroy. That way can add it to map if necessary. Will worry about a better approach later. Would be better to get this PR in sooner.

[AndreasMadsen]

Any thoughts on this?

I think that if the user is interested in the handle object then the user should .enable() accordingly. But I don't have any usecase my for delaying .enable() myself, so I'm likely biased.

Move the parent object to the last argument for create and remove it from destroy.

The words move and create confuses me and the parent was never passed to destroy, do you mean:

init(uid, provider, parentUid, parentHandle);
pre(uid);
post(uid);
destroy(uid);

I think that's fine.

[trevnorris]

@AndreasMadsen My initial implementation of AsyncListener allowed to uniquely track all async paths. So it would've been possible to enable tracking of, say, a single TCP connection. For that to work it would have needed to know the parent of that connection when it was enabled. Would like to have that functionality again going forward, but may be out of scope.

The signatures you outlined looks great. Thanks for your work on this.

[AndreasMadsen]

@trevnorris The general idea makes sense and I can see the relevance of the parent handle. However I'm not entirely convinced that it is necessary to provide the parent handle. That being said I would rather have that we provide too much information that too little.

I've updated the last commit (1db5c06a34a58e6eb3bc2b8c77f3b2236f5e75dd) so it just adds the parent uid without removing the parent handle.

[trevnorris]

@AndreasMadsen Very excellent. CI: https://ci.nodejs.org/job/node-test-pull-request/1531/

CI is currently on lock down until security release is done. Will post results once it finishes.

[trevnorris]

Jenkins failed on the armv8-ubuntu1404, several of the pi2-raspbian-wheezy build. Here are the relevant errors to this PR:

> freebsd102-64
not ok 4 test-async-wrap-check-providers.js
# Not all keys have been used:
# [ 'SHUTDOWNWRAP',
#   'TCPCONNECTWRAP',
#   'TLSWRAP',
#   'UDPSENDWRAP',
#   'WRITEWRAP' ]
# events.js:155
#       throw er; // Unhandled 'error' event
#       ^
# 
# Error: bind EADDRINUSE 0.0.0.0:12346

> pi1-raspbian-wheezy
not ok 1 test-async-wrap-disabled-propagate-parent.js
# 
# assert.js:89
#   throw new assert.AssertionError({
#   ^
# AssertionError: server uid doesn't match parent uid
#     at process.nextTick (/home/iojs/build/workspace/node-test-binary-arm/RUN_SUBSET/1/nodes/pi1-raspbian-wheezy/test/parallel/test-async-wrap-disabled-propagate-parent.js:26:14)
#     at nextTickCallbackWith0Args (node.js:452:9)
#     at process._tickCallback (node.js:381:13)

> pi2-raspbian-wheezy
not ok 1 test-async-wrap-disabled-propagate-parent.js
# 
# assert.js:89
#   throw new assert.AssertionError({
#   ^
# AssertionError: server uid doesn't match parent uid
#     at process.nextTick (/home/iojs/build/workspace/node-test-binary-arm/RUN_SUBSET/3/nodes/pi2-raspbian-wheezy/test/parallel/test-async-wrap-disabled-propagate-parent.js:26:14)
#     at nextTickCallbackWith0Args (node.js:452:9)
#     at process._tickCallback (node.js:381:13)

The failure on freebsd102-64 is not a problem w/ the test.I don't understand how test-async-wrap-disabled-propagate-parent.js could have failed in that way. Will investigate.

[AndreasMadsen]

Hmm that is strange.

I tried running the tests on Raspberry Pi 2 one running ArchLinux (1 error) another running Raspbian (0 errors). I doubt the ArchLinux error is related to this PR:

=== release test-http-1.0 ===
Path: parallel/test-http-1.0
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: undefined == 'hello world\n'
    at response_validator (/home/pi/GitHub/node/test/parallel/test-http-1.0.js:65:12)
    at cleanup [as _onTimeout] (/home/pi/GitHub/node/test/parallel/test-http-1.0.js:20:5)
    at Timer.listOnTimeout (timers.js:92:15)
Command: out/Release/node /home/pi/GitHub/node/test/parallel/test-http-1.0.js

So unfortunately I can't reproduce it.

PS: When I try access https://ci.nodejs.org/job/node-test-pull-request/1531/ it says I don't have "read permissions".

[trevnorris]

@AndreasMadsen CI is currently on lock down b/c of security releases. Hence why I posted the entire error message instead of just the link. If you need any more info let me know.

Running again to make sure this wasn't a fluke

CI: https://ci.nodejs.org/job/node-test-pull-request/1566/

[trevnorris]

The test is still failing. Not sure how this would be possible, since the other tests was succeeding. Will look into it.

[AndreasMadsen]

I'm curious, how are the tests executed? The Jenkins output has the format:

not ok 4 test-async-wrap-check-providers.js

while mine is

=== release test-http-1.0 ===

so I'm guessing it isn't just make test. This could cause a different behaviour, such that another TCPWRAP is created before the one that is related to the server. This could be caused by createWritableStdioStream, where I might get a TTY stream but Jenkins produces a TCP stream. – I'm just guessing.

[Trott]

@AndreasMadsen I believe Jenkins runs make test-ci rather than make test.

[AndreasMadsen]

@Trott thanks, unfortunately make test-ci also passes.

Is there a way to force node to use TCP wrap?

• node script.js will use TTY
• node script.js | less will use PIPE
• node script.js &> out will use FILE

But I actually don't know when TCP is used :/

[Trott]

@AndreasMadsen CI is unlocked now so you should be able to review the results and the full log to (hopefully) see exactly how to recreate the issues that are showing up there.

One thing I notice is that test-async-wrap-check-providers didn't fail on Raspberry Pi devices, so your non-failure would be consistent with that.

You can review the tests that failed at https://ci.nodejs.org/job/node-test-binary-arm/972/

[thefourtheye]

Better to have assert.strictEqual here.