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

There are a lot of failures for test-http-pipeline-flood.js. Though this PR shouldn't affect this PR /cc @indutny

One more time to see if the previous was a fluke

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

All but one failure was for test-http-pipeline-flood. Most failures had the usual
AssertionError: Unexpected data received. Though the following had deviations from this:

linux-fips: Error: write ECONNRESET

freebsd: Error: write EPIPE

win10-vcbt2015: FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory

win2012r2-vs2015: # TIMEOUT

@rvagg Maybe you have an idea how to proceed, or maybe something is going on w/ Jenkins?

@indutny confirmed that switching to MakeCallback() causes the flood protection to fail. figuring out why now.

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

@indutny I've added two AsyncCallbackScopes to prevent the nextTickQueue from being processed. This was causing the state of the parser and socket to be altered, whereas they weren't before. The state change allowed the socket to continually consume new requests.

The reason the pipeline flood fix worked before was because the entire packet of requests was essentially processed synchronously. But calling MakeCallback processes the nextTickQueue and changes the state of the http stream. Below are some of the notes I took while investigating:

In the case of parserOnMessageComplete(), if AsyncCallbackScope is not
used then readStart(parser.socket) will run socket.resume(). This is
because !socket._paused returns true, whereas it shouldn't.

The reason socket._paused = false is because socketOnDrain()
(lib/_http_server.js) is called if the AsyncCallbackScope isn't added.

In the end I believe this fix is the simplest, and keeps the parser operating like it did before.

Ah, yeah. Nice catch! I was going to take a look at it, but you beat me 😉

LGTM if it works

Thanks much. CI is all green. Landed in a7e49c8.

@trevnorris ... are you going to want to pull this back into v4 at all? If not, can you mark it don't land?

I think the goal is still to backport all the asyncwrap stuff?

As @Fishrock123 said, all the AsyncWrap changes will need to be back ported if it is to maintain compatibility.

Oh hey all. It looks like this commit has created a regression on v5.7.1. Specifically throwing inside of a call back to an http.get request is not throwing :sad:

Here is a gist for the test I was using https://gist.github.com/TheAlphaNerd/6615a27684deb682dfe7

You will notice that the setTimeout is commented out... adding set timeout actually fixes this problem 😢

Based on the regression created by this change I'm going to assume we are not going to land it on v4.

@trevnorris let me know if the plans for v4 and asyncwrap changes

I think this probably needs to find its way into LTS but can do so after a longer wait, it fills in the AsyncWrap picture more and the discrepancy between LTS and Stable will be non-trivial if it doesn't land there, and we're getting close to deciding on "official" support for AsyncWrap too.

@thealphanerd I'm going to switch back to lts-watch but we need good settling time for this given the regression.

@rvagg SGTM. I'll remove the don't land label from the regression fix

@thealphanerd The regression was fixed in 3521b05. As @rvagg said this is not absolutely necessary for backport, but it shouldn't be a problem to backport with the fix.