Active `Domain` context is being leaked between server requests.

[m0uneer]

Version: v6.14.3 and v10.5.0
Express Version: 4.16.4
Platform:
Linux m-pc 4.13.0-41-generic # 46~16.04.1-Ubuntu SMP Thu May 3 10:06:43 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

Steps (Generating a simple Express app):

• $ npm install express-generator -g
• $ express --view=pug myapp
• $ npm i
• Removing unneeded dependencies to keep it as simple and free of external dependencies.
• Adding some domain implementations.

var domain = require('domain');

// Patching global promise in response to https://github.com/nodejs/node-v0.x-archive/issues/8648
class PatchedPromise extends Promise { 
  // ...
}

Promise = PatchedPromise;

// Adding the `__awaiter` method to support old nodejs versions
var __awaiter = function (thisArg, _arguments, P, generator) {
  // ...
};

var express = require('express');
var app = express();

app.use(function(req, res, next) {
  let activeDomain = domain.active ? domain.active : domain.create();
  activeDomain.run(() => {
    if (domain.active.trap) {
      throw new Error('Context Leaked!');
    }

    domain.active.trap = {};
    __awaiter(this, void 0, void 0, function* () {
      try {
        yield new Promise(() => {
          throw new Error('test');
        });
      }
      catch (err) {
        next(err);
      }
    });
  });
});

module.exports = app;

• $ npm start
• Send a POST request to /.

Expected:
Always return Error('test').

Actual:
Only first request returns Error('test') and all the comming requests return Error('Context Leaked!')

A link to a repo to reproduce:
https://github.com/m0uneer/domain-promise-problem

[ofrobots]

Hi @m0uneer. Is this a regression from previous versions of node?

The domains module has a lot of edge cases and is deprecated. I would recommend using async_hooks API instead. There are some userspace modules (e.g. cls-hooked) that provide a user-friendly API on top of async-hooks.

[misterdjules]

This seems like it could be the server counterpart of #25456.

Basically, it seems likely that in the repro above, the parser of the first HTTP request is reused from one request to the next but its domain is not reset/re-attached after the first request.

I would think a fix similar to what's been done in #25459 for HTTP clients could fix this issue.

[misterdjules]

Actually, scratch what I mentioned above. It looks like the issue is related to the implementation of promises that you use in that repo. It seems that implementation doesn't exit from the domain it entered when it throws an error?

For instance, exiting the current domain with process.domain.exit() in the catch handler at https://github.com/m0uneer/domain-promise-problem/blob/ec38201a07b7b6bffeb6c7abdac4c57998ea7077/app.js#L57-L59 fixes this issue.

I'm not sure if that would be the right fix for your use case or if it'd be possible to exit the current domain in your promises implementation for a more general solution.

In other words. so far this doesn't look like a bug in node but I may be missing something.

[m0uneer]

Thanks for your responses @misterdjules . But I wonder, why the bug occurs with POST requests only?

[misterdjules]

@m0uneer I'm able to reproduce the problem you described by sending GET requests.

[m0uneer]

@misterdjules , @ofrobots , the original code is written in TS and built for ES5.
The Promise patch is created by me to fix the mentioned issue where the domain.active becomes undefined inside the then and catch of native Promises.

I copied pasted the compiled code and edited it to make the repo as simple as possible and this explains why the __awaiter and generators* are over there.

I tried the process.domain.exit(); before creating this issue inside my real code and didn't work so I opened this ticket but as @misterdjules mentioned it works for the current code above. So I'll check my code again for the missing thing and come back to you with any new findings.

[misterdjules]

I think your problem essentially boils down to this:

const domain = require('domain')
const d = domain.create()

function boom() {
  throw new Error('boom')
}

try {
  d.bind(boom)()
} catch (err) {
  console.log(domain._stack)
}

Every time a function is bound to a domain, if that function throws and the thrown error is caught, the domain that was bound to this function won't be exited.

When running the promise executor in your promises implementation as bound to the domain, that domain is never exited when the promise's executor throw, so it's left on the stack even after that http handler completes its execution.

[m0uneer]

@misterdjules exactly! Now it makes sense why in my case process.domain.exit() does not work because I need to call it multiple times and not just once due to the multiple Promise calls inside my actual code.

    __awaiter(this, void 0, void 0, function* () {
      try {
        var p1 = new Promise(() => {
          throw new Error('test');
        });

        yield new Promise(() => {
          throw new Error('test');
        });
      }
      catch (err) {
        // Two call are needed to fix the problem.
        process.domain.exit();
        process.domain.exit();
        next(err);
      }
    });

Well, I'm looking now for a fix to my Promise patch and I will let you know when I succeeded.

[misterdjules]

@m0uneer Does it sound reasonable then if we close this issue as it doesn't seem to be a bug with domains? We can still discuss it here though if we close it :)

[m0uneer]

@misterdjules I tried to find a solution that fits my application but all solutions lead to destroying the context that I use later in the code.

So, I ended up looping on the domain._stack at the end of my request and exiting them.

domain._stack.forEach(stackedDomain => stackedDomain.exit());

Do you think there is any better solution?