AsyncWrap: TCPWrap created before MakeCallback

[AndreasMadsen]

When creating a TCP server, the TCP handles there holds the socket is created in C++ land. The socket handle is then send to the onconnection server handle event using MakeCallback.

code link: https://github.com/nodejs/node/blob/master/src/tcp_wrap.cc#L259L272

Because the socket handle is created before MakeCallback, the async_hooks_init_function for the socket handle is called before async_hooks_before_function for the server handle. This makes it impossible to trace back the origin of the socket handle.

dprof dump: http://bl.ocks.org/AndreasMadsen/raw/da3adb776bdfcf30f97f/ - Notice the free hanging blue line, which has an origin from nowhere.

edit: the link is only tested in Chrome, so here is a picture

The visualization/test was done with the following code:

'use strict';
require('../../dprof.js');

const net = require('net');

const server = net.createServer(function (socket) {
  socket.end('hallo world');
});

server.listen(0, 'localhost', function () {
  const addr = server.address();
  const socket = net.connect(addr.port, addr.host, function () {
    socket.once('readable', function () {
      socket.read();
      socket.once('readable', server.close.bind(server));
    });
  });
});

This issue appears to be caused by 7dde95a8bd. I agree that is is wired to call async_hooks_before_function in the AsyncWrap constructor, particularly in this case because it suggests that the creation of the TCPWrap object and onconnection happens in two different async turns.

However I'm not sure what the proper fix would be.

/cc @trevnorris

[trevnorris]

The timing of all three callbacks are correct, but this an obvious deficiency in functionality. What if the parent was passed as an argument to the async_hook_init_function() callback? I feel like that would be an appropriate way to propagate that information. Thoughts?

[AndreasMadsen]

I did actually implement that locally. The only issue is that async_hook_init_function() for the socket handle is called before async_hooks_before_function() for the server handle, which makes it unclear what "callback" the socket belongs to.

see: http://bl.ocks.org/AndreasMadsen/raw/4354435e99e8f7c35a5d/
Note: red is when a callback is being executed and blue is when the handle is waiting.

One could properly "fix" the timings for async_hooks_before_function() to look for previous async_hook_init_function() calls, but it seams like a complex solution from a userland perspective. But it may be the best the solution.

[trevnorris]

which makes it unclear what "callback" the socket belongs to.

This I hadn't considered. Was only thinking about making sure information about handle "ownership" propagated properly.

see: http://bl.ocks.org/AndreasMadsen/raw/4354435e99e8f7c35a5d/

Can't ATM. Page dies w/ a JS error showing in the console.

Could you write a basic test case w/ comments on what you'd expect to happen? I'm not fully understanding if you're using some sort of global state to keep track of what init callback are called while the before/after callbacks are called. Or what you're using that for.

[AndreasMadsen]

Can't ATM. Page dies w/ a JS error showing in the console.

Yeah, sorry. At this stage it only works in Chrome. Here is a picture:

I'm not fully understanding if you're using some sort of global state to keep track of what init callback are called while the before/after callbacks are called. Or what you're using that for.

That is exactly what I'm doing. Here is the code If you are interested: https://github.com/AndreasMadsen/dprof/blob/master/dprof.js#L82L94

Could you write a basic test case w/ comments on what you'd expect to happen?

Sure, shouldn't be a problem.

[AndreasMadsen]

Here is a simple test case for what I initially expected to happen: https://gist.github.com/AndreasMadsen/ca42b0e7477d7209d4d7

[trevnorris]

Thanks for the example and reproduction. What about doing something like so instead:

function asyncInit(parent) {
  if (parent)
    parent.owner._dprofState.add(this);
}

I'm making the assumption that _dprofState is a Set(). Reason for parent.owner is because parent is actually the _handle of the user facing JS object created. Now, if parent exists but parent.owner does not then there's probably at least an inconsistency that should be addressed.

[trevnorris]

Now, I believe there's a bug properly propagating the parent, but if that sort of solution would work for you then I'll investigate getting that working sooner.

[AndreasMadsen]

Reason for parent.owner is because parent is actually the _handle of the user facing JS object created.

Hmm, I'm not sure I understand what .owner is.

PS: _dprofState is not a Set(). It is a node in a tree, the .add methods ads a new child to that node and returns the new node (child). But I don't think it is important for your reasoning.

[trevnorris]

@AndreasMadsen When you run:

var server = net.createServer();

It returns a JS object. Once you call listen it internally does:

this._handle = new TCP();
this._handle.owner = this;

So they reference each other. What we can pass to JS from native is the _handle. From which you can retrieve the server object, and add() the information you want. Think that's worth a try?

[AndreasMadsen]

Okay, I think we are on the same page then. But adding a property in async_hook_init_function to handle or handle.owner is information equivalent. So I don't see how this solves the issue: "what callback does the new handle belong to?".

There is a work around from the userland perspective. Assuming that the new handle belongs to the next callback of the parent handle, one could keep track of child handles and then backtrack what has happened. But it is not ideal.

I did already try it, this is what I'm talking about in #2986 (comment).

[trevnorris]

So I don't see how this solves the issue: "what callback does the new handle belong to?".

I think this is where the wires are mostly crossed. Internally it doesn't belong to any callback. It belongs to a handle instance. The handle just happens to call a callback to let us know that another handle has been created. But this isn't consistent.

For example, with the http module the user isn't alerted that a connection has been received until all the headers have been parsed. But async wrap will let you know the moment a connection was received. Likewise, any ReqWrap created because of a handle won't be called within the scope of the parent's callback. It has its own callback that will be called. So ownership must be determined by notifying which handle is responsible for another's existence.

[AndreasMadsen]

Internally it doesn't belong to any callback. It belongs to a handle instance. The handle just happens to call a callback to let us know that another handle has been created. But this isn't consistent.

What I'm trying to say is that I would like that to be consistent. But maybe we should take that as a separate issue and just ensure that the handle context is always known for now.

Adding the parent handle as an argument as I have done in AndreasMadsen@04d4fad, is however not enough to always know the context.

For example in the HTTP case you mentioned, I'm still missing the handle context in a few cases. It appears to be because the HTTPParser doesn't inherit from AsyncWrap at all. (e.g. https://github.com/nodejs/node/blob/master/src/node_http_parser.cc#L289)

I guess it makes sense, because the HTTPParser is invoked though a pool of parsers, so the handle parent changes, but it is a big issue in terms of handle context. However you properly know more about.

example and result:

'use strict';
const http = require('http');

const server = http.createServer(function (req, res) {
  res.end('hallo world');
});

server.listen(0, 'localhost', function () {
  const addr = server.address();
  const req = http.get(`http://${addr.address}:${addr.port}`, function (res) {
    res.resume();
    res.once('end', server.close.bind(server));
  });
});

dprof dump: http://bl.ocks.org/AndreasMadsen/raw/e2a9fb08ee6813c5663a/

[trevnorris]

is however not enough to always know the context.

Not trying to say that it is enough. Simply that both approaches are necessary because neither can capture all cases. I agree that consistency would be optimal, but it's technically not possible.

The reason I implemented passing the parent is so you can capture which Server an incoming Socket came from: https://github.com/nodejs/node/blob/v4.1.1/src/tcp_wrap.cc#L258-L260
I removed the before/after callbacks on the parent in the AsyncWrap constructor because they were meant for MakeCallback. So the alternative was to add 2 more callbacks, or pass the parent. Properly implementing the callbacks would have made the implementation more tedious and error prone because snippets like the one above would need to be wrapped. So it seemed the best solution was to simply pass the parent to the init callback.

I guess it makes sense, because the HTTPParser is invoked though a pool of parsers

Its' because HTTPParser is synchronous and simply leaches onto the incoming data stream and parses it immediately.

[AndreasMadsen]

#3216 solves this particular issue.