Fix async wrap http

[indutny]

R=@trevnorris or @bnoordhuis

[trevnorris]

Doesn't address node::MakeCallback(). Also the prevention of reentry should technically be addressed by the in_tick() call below. Except there's a bug using SetVerbose() which never allows those to run in case of an error. I have a work in progress fix #4507 but some tests are still failing.

Switching to use AsyncWrap and adding the HTTP provider is nice. Sure some developers will enjoy seeing that information.

[indutny]

Preemption won't be fixed by in_tick(), as far as I can see. in_tick guards only tick callback, and won't guard any other cbs invoked.

[trevnorris]

@indutny Do you mean that neither the domain nor async callbacks should run in the case of nested MakeCallback() calls?

[indutny]

Mmm... I don't know, technically there is no point in reporting it, or is there?

[trevnorris]

mind changing this to HTTPPARSER (that's exactly what it's being used for right?) and also ordering it alphabetically?

[trevnorris]

Even if the MakeCallback call is nested, I'm not sure it's correct that the domain enter() function doesn't run. It should still effectively be it's own execution stack (if that makes sense), right?

[indutny]

I have totally no idea how it should work. cc @misterdjules maybe?

[trevnorris]

@indutny @misterdjules responded in #4507 (comment) in more detail about how domains should work in MakeCallback. I changed my PR to match his information, and provided an explanation below.

@ofrobots I'm not sure AsyncCallbackScope is needed. Going to do some testing against my linked PR above and see if the issue has been resolved.

[misterdjules]

@trevnorris My comment at #4507 (comment) was more a set of questions that came up to mind when reading the changes in #4507 than an authoritative answer. I still haven't had the time to make up my mind on what is the proper way to answer these questions.

[misterdjules]

@indutny @trevnorris I would think that entering the domain in a nested synchronous node::*::MakeCallback call is useless, since it would enter the same domain as its caller.

The reason why it's useful to call Domain.prototype.enter in the node::*::MakeCallback root call is that it's always (at least for now) a call that is made asynchronously, and thus at that point all active domains have exited, including the domain within which the callback was scheduled. That domain needs to be reentered.

For instance in the following code snippet:

var domain = require('domain');

var d = domain.create();
d.run(function() {
  setImmediate(foo() { throw new Error('boom'); });
});

by the time foo is called by node::MakeCallback, domain d exited, and needs to be re-entered. If the node::MakeCallback call that called foo was re-entered, that domain would already be on the domains stack and would already be the active domain, so there would be no need to enter it.

However, if for some reason you make the decision to re-enter the domain in that case , it needs to be exited. Basically it's either entering and exiting in nested calls, or not doing any of them.

I hope that makes sense and that helps.

[trevnorris]

@indutny PING PING PING

[AndreasMadsen]

@trevnorris took over this PR and created #5419, it landed a month ago.