-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
[v8.x] http: attach reused parser to correct domain #25459
Closed
misterdjules
wants to merge
3
commits into
nodejs:v8.x-staging
from
misterdjules:reused-parser-domain
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
const domain = require('domain'); | ||
const http = require('http'); | ||
|
||
const agent = new http.Agent({ | ||
// keepAlive is important here so that the underlying socket of all requests | ||
// is reused and tied to the same domain. | ||
keepAlive: true | ||
}); | ||
|
||
const reqSockets = new Set(); | ||
|
||
function performHttpRequest(opts, cb) { | ||
const req = http.get({ port: server.address().port, agent }, (res) => { | ||
if (!req.socket) { | ||
console.log('missing socket property on req object, failing test'); | ||
markTestAsFailed(); | ||
req.abort(); | ||
cb(new Error('req.socket missing')); | ||
return; | ||
} | ||
|
||
reqSockets.add(req.socket); | ||
|
||
// Consume the data from the response to make sure the 'end' event is | ||
// emitted. | ||
res.on('data', function noop() {}); | ||
res.on('end', () => { | ||
if (opts.shouldThrow) { | ||
throw new Error('boom'); | ||
} else { | ||
cb(); | ||
} | ||
}); | ||
|
||
res.on('error', (resErr) => { | ||
console.log('got error on response, failing test:', resErr); | ||
markTestAsFailed(); | ||
}); | ||
|
||
}).on('error', (reqErr) => { | ||
console.log('got error on request, failing test:', reqErr); | ||
markTestAsFailed(); | ||
}); | ||
|
||
req.end(); | ||
} | ||
|
||
function performHttpRequestInNewDomain(opts, cb) { | ||
const d = domain.create(); | ||
d.run(() => { | ||
performHttpRequest(opts, cb); | ||
}); | ||
|
||
return d; | ||
} | ||
|
||
const server = http.createServer((req, res) => { | ||
res.end(); | ||
}); | ||
|
||
server.listen(0, common.mustCall(function() { | ||
const d1 = performHttpRequestInNewDomain({ | ||
shouldThrow: false | ||
}, common.mustCall((firstReqErr) => { | ||
if (firstReqErr) { | ||
markTestAsFailed(); | ||
return; | ||
} | ||
// We want to schedule the second request on the next turn of the event | ||
// loop so that the parser from the first request is actually reused. | ||
setImmediate(common.mustCall(() => { | ||
const d2 = performHttpRequestInNewDomain({ | ||
shouldThrow: true | ||
}, (secondReqErr) => { | ||
// Since this request throws before its callback has the chance | ||
// to be called, we mark the test as failed if this callback is | ||
// called. | ||
console.log('second request\'s cb called unexpectedly, failing test'); | ||
markTestAsFailed(); | ||
}); | ||
|
||
// The second request throws when its response's end event is | ||
// emitted. So we expect its domain to emit an error event. | ||
d2.on('error', common.mustCall((d2err) => { | ||
server.close(); | ||
if (reqSockets.size !== 1) { | ||
console.log('more than 1 socket used for both requests, failing ' + | ||
'test'); | ||
markTestAsFailed(); | ||
} | ||
}, 1)); | ||
})); | ||
})); | ||
|
||
d1.on('error', (d1err) => { | ||
// d1 is the domain attached to the first request, which doesn't throw, | ||
// so we don't expect its error handler to be called. | ||
console.log('first request\'s domain\'s error handler called, ' + | ||
'failing test'); | ||
markTestAsFailed(); | ||
}); | ||
})); | ||
|
||
function markTestAsFailed() { | ||
if (server) { | ||
server.close(); | ||
} | ||
process.exitCode = 1; | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an assertion that checks that the connection is the same for all requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
Instead of an assertion, which can throw an exception and thus trigger a different code path with regards to domain error handling that would make this test really difficult to write, do you mind if, as for other failure cases in this test, we set the exit code to 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@misterdjules Yes, that sounds good to me as well :)