From 1d862610f85aa3ee8ca2e7a4f120794eff7133c4 Mon Sep 17 00:00:00 2001
From: Julien Gilli <jgilli@netflix.com>
Date: Fri, 11 Jan 2019 15:28:08 -0800
Subject: [PATCH] http: attach reused parser to correct domain

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: https://github.com/nodejs/node/issues/25456

PR-URL: https://github.com/nodejs/node/pull/25459
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
---
 lib/_http_client.js                           |   9 +-
 .../parallel/test-http-parser-reuse-domain.js | 112 ++++++++++++++++++
 2 files changed, 115 insertions(+), 6 deletions(-)
 create mode 100644 test/parallel/test-http-parser-reuse-domain.js

diff --git a/lib/_http_client.js b/lib/_http_client.js
index 7b5798fe021a87..0dc8da0f4f6c5e 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -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) {
@@ -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;
diff --git a/test/parallel/test-http-parser-reuse-domain.js b/test/parallel/test-http-parser-reuse-domain.js
new file mode 100644
index 00000000000000..fd164f47d1e007
--- /dev/null
+++ b/test/parallel/test-http-parser-reuse-domain.js
@@ -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;
+}