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.

Sign up for GitHub

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

Jump to bottom

http: guard against failed sockets creation #13839

Merged
merged 1 commit into from
Jul 3, 2017
Merged

http: guard against failed sockets creation #13839

merged 1 commit into from
Jul 3, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Jun 21, 2017 •
edited
Loading

Fixes: #13045
Fixes: #13831
Ref: #13352
Ref: #13548 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http,async_hooks

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 21, 2017
@refack
Copy link
Contributor Author

refack commented Jun 21, 2017 •
edited
Loading

@nodejs/async_hooks silly question, what do I pass to nextTick if I don't have a new _handle? Where's the "ambient" asyncId?

@refack refack changed the title http: guard against failed sockets creating http: guard against failed sockets creation Jun 21, 2017
mscdex
mscdex reviewed Jun 21, 2017
View reviewed changes
lib/_http_agent.js Outdated
@@ -168,8 +168,10 @@ Agent.prototype.addRequest = function addRequest(req, options, port/*legacy*/,
// we have a free socket, so use that.
var socket = this.freeSockets[name].shift();
// Assign the handle a new asyncId and run any init() hooks.
socket._handle.asyncReset();
socket[async_id_symbol] = socket._handle.getAsyncId();
if (socket._handle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this check is necessary. It's only the socket creation that's an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.
There's still the possibility that createSocket will be overridden to create sockets that do not have socket._handle.asyncReset (this is a method that only tcp_wrap and tls_wrap have). I should have guarded for that (not simply for the existence of _handle), but now I don't think that's an interesting use-case.

@refack refack added the async_hooks Issues and PRs related to the async hooks subsystem. label Jun 21, 2017
@refack
Copy link
Contributor Author

refack commented Jun 23, 2017

Ping @nodejs/async_hooks

@refack refack mentioned this pull request Jun 25, 2017
Closed
trevnorris
trevnorris approved these changes Jun 29, 2017
View reviewed changes
Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

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

Two minor things. One is to change usage of nextTick() the other is a comment about unifying the way tests fail inside a Promise. I don't think I'd consider either a blocker.

lib/_http_agent.js Outdated

const asyncId = socket && socket._handle && socket._handle.getAsyncId();
if (asyncId) {
nextTick(asyncId, emitErr);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can pass null here and the internal nextTick() will grab the correct id.

test/parallel/test-http-agent.js Outdated
}).catch((e) => {
// This is currently the way to fail a test with a Promise
console.error(e);
process.exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw the fatalError() code in lib/async_hooks.js into test/common (https://github.com/nodejs/node/blob/v8.1.3/lib/async_hooks.js#L54-L68)? This way there's a standardized way of failing. e.g.

const { promiseFatalError } = require('../common'):

Promise.reject(42).catch(promiseFatalError);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh, fancy 🎩!
I like!
New PR coming.

@refack refack mentioned this pull request Jul 2, 2017
Closed
@refack
Copy link
Contributor Author

refack commented Jul 3, 2017

CI: https://ci.nodejs.org/job/node-test-commit/10899/

This was referenced Jul 3, 2017
Closed
Closed
@refack
Copy link
Contributor Author

refack commented Jul 3, 2017

CI: https://ci.nodejs.org/job/node-test-commit/10919/

@refack refack merged commit 4b276e9 into nodejs:master Jul 3, 2017
@refack refack deleted the http-agent-band-aid branch July 3, 2017 19:17
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 3, 2017
@addaleax
http: guard against failed sockets creation
45a9c58 
PR-URL: nodejs#13839
Fixes: nodejs#13045
Fixes: nodejs#13831
Refs: nodejs#13352
Refs: nodejs#13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
@addaleax addaleax mentioned this pull request Jul 3, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@refack @addaleax
http: guard against failed sockets creation
2ce0de1 
PR-URL: #13839
Fixes: #13045
Fixes: #13831
Refs: #13352
Refs: #13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
@refack refack mentioned this pull request Jul 12, 2017
Closed
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@refack @addaleax
http: guard against failed sockets creation
c652845 
PR-URL: #13839
Fixes: #13045
Fixes: #13831
Refs: #13352
Refs: #13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@refack @Fishrock123
http: guard against failed sockets creation
959bdfd 
PR-URL: #13839
Fixes: #13045
Fixes: #13831
Refs: #13352
Refs: #13548 (comment)
Reviewed-By: Trevor Norris <[email protected]>
@refack refack mentioned this pull request Jul 22, 2017
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@trevnorris trevnorris trevnorris approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
5 participants
@refack @mscdex @trevnorris @AndreasMadsen @nodejs-github-bot