Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http_parser: use MakeCallback #5419

Merged
merged 1 commit into from
Mar 1, 2016
Merged

Conversation

trevnorris
Copy link
Contributor

Make HTTPParser an instance of AsyncWrap and make it use
MakeCallback. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.

Fix: #4416

This PR overrides #4509.

R= @indutny
R= @bnoordhuis

@trevnorris trevnorris added http Issues or PRs related to the http subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 24, 2016
@@ -104,6 +106,7 @@ function expectBody(expected) {
assert.throws(function() {
parser.execute(request, 0, request.length);
}, Error, 'hello world');
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Why did you disable this test in your original PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it pass now?

If I remember it right it just killed the process with uncaught exception last time (FatalException or whatever).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does on my machine. Running CI again to see if it does.

@trevnorris
Copy link
Contributor Author

@trevnorris
Copy link
Contributor Author

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

@trevnorris
Copy link
Contributor Author

One more time to see if the previous was a fluke

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

@trevnorris
Copy link
Contributor Author

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?

@trevnorris
Copy link
Contributor Author

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

@trevnorris
Copy link
Contributor Author

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.

@indutny
Copy link
Member

indutny commented Mar 1, 2016

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

@indutny
Copy link
Member

indutny commented Mar 1, 2016

LGTM if it works

Make `HTTPParser` an instance of `AsyncWrap` and make it use
`MakeCallback`. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.

Additional uses of `AsyncCallbackScope` are necessary to prevent
improper state from progressing that triggers failure in the
test-http-pipeline-flood.js test. Optimally this wouldn't be necessary,
but for the time being it's the most sure way to allow operations to
proceed as they have.

Fix: nodejs#4416
PR-URL: nodejs#5419
Reviewed-By: Fedor Indutny <[email protected]>
@trevnorris trevnorris merged commit a7e49c8 into nodejs:master Mar 1, 2016
@trevnorris
Copy link
Contributor Author

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

@trevnorris trevnorris deleted the parser-use-mc branch March 1, 2016 09:41
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

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

@Fishrock123
Copy link
Contributor

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

Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
Make `HTTPParser` an instance of `AsyncWrap` and make it use
`MakeCallback`. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.

Additional uses of `AsyncCallbackScope` are necessary to prevent
improper state from progressing that triggers failure in the
test-http-pipeline-flood.js test. Optimally this wouldn't be necessary,
but for the time being it's the most sure way to allow operations to
proceed as they have.

Fix: #4416
PR-URL: #5419
Reviewed-By: Fedor Indutny <[email protected]>
@trevnorris
Copy link
Contributor Author

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

@MylesBorins
Copy link
Contributor

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 😢

@MylesBorins
Copy link
Contributor

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

@rvagg
Copy link
Member

rvagg commented Mar 10, 2016

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.

@MylesBorins
Copy link
Contributor

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

@trevnorris
Copy link
Contributor Author

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

MylesBorins pushed a commit that referenced this pull request Jun 7, 2016
Make `HTTPParser` an instance of `AsyncWrap` and make it use
`MakeCallback`. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.

Additional uses of `AsyncCallbackScope` are necessary to prevent
improper state from progressing that triggers failure in the
test-http-pipeline-flood.js test. Optimally this wouldn't be necessary,
but for the time being it's the most sure way to allow operations to
proceed as they have.

Ref: #7048
Fix: #4416
PR-URL: #5419
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
Make `HTTPParser` an instance of `AsyncWrap` and make it use
`MakeCallback`. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.

Additional uses of `AsyncCallbackScope` are necessary to prevent
improper state from progressing that triggers failure in the
test-http-pipeline-flood.js test. Optimally this wouldn't be necessary,
but for the time being it's the most sure way to allow operations to
proceed as they have.

Ref: #7048
Fix: #4416
PR-URL: #5419
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 28, 2016
Make `HTTPParser` an instance of `AsyncWrap` and make it use
`MakeCallback`. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.

Additional uses of `AsyncCallbackScope` are necessary to prevent
improper state from progressing that triggers failure in the
test-http-pipeline-flood.js test. Optimally this wouldn't be necessary,
but for the time being it's the most sure way to allow operations to
proceed as they have.

Ref: #7048
Fix: #4416
PR-URL: #5419
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Make `HTTPParser` an instance of `AsyncWrap` and make it use
`MakeCallback`. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.

Additional uses of `AsyncCallbackScope` are necessary to prevent
improper state from progressing that triggers failure in the
test-http-pipeline-flood.js test. Optimally this wouldn't be necessary,
but for the time being it's the most sure way to allow operations to
proceed as they have.

Ref: #7048
Fix: #4416
PR-URL: #5419
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants