Skip to content

Commit

Permalink
http: attach reused parser to correct domain
Browse files Browse the repository at this point in the history
Reused parsers can be attached to the domain that corresponds to the
active domain when the underlying socket was created, which is not
necessarily correct.

Instead, we attach parsers to the active domain if there is one when
they're reused from the pool.

Refs: #25456

PR-URL: #25459
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
Julien Gilli authored and BethGriggs committed Mar 20, 2019
1 parent d6ffabc commit 1d86261
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 6 deletions.
9 changes: 3 additions & 6 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -509,12 +509,6 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
var socket = this.socket;
var req = socket._httpMessage;

// propagate "domain" setting...
if (req.domain && !res.domain) {
debug('setting "res.domain"');
res.domain = req.domain;
}

debug('AGENT incoming response!');

if (req.res) {
Expand Down Expand Up @@ -629,6 +623,9 @@ function tickOnSocket(req, socket) {
req.socket = socket;
req.connection = socket;
parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]);
if (process.domain) {
process.domain.add(parser);
}
parser.socket = socket;
parser.incoming = null;
parser.outgoing = req;
Expand Down
112 changes: 112 additions & 0 deletions test/parallel/test-http-parser-reuse-domain.js
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;
}

0 comments on commit 1d86261

Please sign in to comment.