reused http parser can be associated to incorrect domain

[misterdjules]

• Version: Latest v8.x, and possibly all earlier versions supporting HTTP parser reuse and domains.
• Platform: All platforms.
• Subsystem: domain, http.

The following program exits with an exit code of 1 when run with node built from the tip of the v8.x branch:

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
});

function performHttpRequest(opts, cb) {
    const req = http.get({port: server.address().port, agent}, (res) => {
        // 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) => {
            process.exit(1);
        });

    }).on('error', reqErr => {
        process.exit(1);
    });

    req.end();
}

function performHttpRequestInNewDomain(opts, cb) {
    const d = domain.create();
    d._id = opts.id;
    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) => {
        // 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.
                process.exit(1);
            });

            // 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();
            }, 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.
        process.exit(1)
    });
}));

What this programs above does is:

1. Creating one outgoing http request to a local http server, (let's call it request 1), in the context of a newly created domain (let's call it domain 1). This request uses an agent that has keepAlive set to true. The purpose of using an agent and setting keepAlive to true is to force the second request (see below) to reuse the same HTTP parser instance as this one.

2. When request 1 completes, a second request is created using the same http agent but in the context of a separate domain (let's call it domain 2). In the response's 'end' event listener of that request, an uncaught error is thrown.

What we expect is that the domain in which the second request was created (domain 2) would handle that uncaught exception, but what happens is that instead the first domain handles it.

I think that the cause of this problem is that, when a HTTP parser is being reused from the pool, it's not re-attached to the active domain.

There is code in parserOnIncomingClient that seems to be responsible for attaching the parser to the correct domain, but it does so only if that parser wasn't already attached to a previous domain. That doesn't work when a parser is reused.

I think the following patch would fix this issue in a way that is more robust:

diff --git a/lib/_http_client.js b/lib/_http_client.js
index 7b5798fe02..0dc8da0f4f 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;

This problem doesn't surface in node 10 because when a parser is reused, its underlying async resource is reinitialized and the proper async hooks events are emitted, which allows the domains state machine to associate the parser to the active domain.

[misterdjules]

Also, thanks to @cprussin and @mridgway for finding this problem and identifying the issue in the domain implementation in the first place!

[BridgeAR]

Node.js v8.x has reached the end-of-life and won't receive any fixes anymore. I am closing this since this issue seems to only apply to Node.js v8.x. If it does in fact also apply to newer versions, please leave a comment so that this can be reopened (@misterdjules I only saw a fix specifically for v8.x?).

No matter if you run into this issue or not, please update to a newer Node.js version in case you still use v8.x.