express.json() breaks AsyncLocalStorage in node 14

[OllieNilsen]

If express.json() is added between middleware which sets asyncLocalStorage and code which retrieves it, the storage goes missing on requests with a body, e.g. POST requests.

Works

import {AsyncLocalStorage} from 'async_hooks';
import {json} from 'express';

const asyncLocalStorage = new AsyncLocalStorage();
const setContext = (req,res,next) => ayncLocalStorage.run('foo', next);

// ...
app.use(
  json(),
  setContext,
  (req, res, next) => {
    asyncLocalStorage.getStore(); // foo
  });

Fails

// ...
app.use(
  setContext,
  json(),
  (req, res, next) => {
    asyncLocalStorage.getStore(); // undefined <<<===
  });

[dougwilson]

This is because ayncLocalStorage.run('foo', next); only works when your following middleware are sync, which body parsing is not, as it needs to wait for the body to arrive before continuing.

https://nodejs.org/dist/latest-v14.x/docs/api/async_hooks.html#async_hooks_asynclocalstorage_run_store_callback_args

This methods runs a function synchronously within a context and return its return value. The store is not accessible outside of the callback function or the asynchronous operations created within the callback.

[wesleytodd]

I think we should consider support for these hooks being passed along, there is an api which enables this and I think we should make it a priority to use going forward. I think the pattern is:

function createMW (opts) {
  function ourAsyncMW (req, res, next) {
    const currentStore = opts.asyncLocalStorage && opts.ayncLocalStorage.getStore()
    if (currentStore) {
      opts.ayncLocalStorage.run(currentStore, () => ourAsyncWork(next))
    } else {
      ourAsyncWork(next)
    }
  }
}

@vdeturckheim do I have the above right for when we register callbacks on streams?

So anywhere we register a callback for async work we would need to propagate the storage. It is actually pretty easy and I hacked this example to work with only a few lines in body-parser (other than passing the option, this was it):

  // read.js
  var currentStore = opts.asyncLocalStorage && opts.asyncLocalStorage.getStore()
  if (currentStore) {
    getBody(stream, opts, (err, body) => opts.asyncLocalStorage.run(currentStore, onBody, err, body))
  } else {
    getBody(stream, opts, onBody)
  }

Happy to submit this PR to body-parser if you think this seems right.

[dougwilson]

The main issue is this is exactly the same issue domains had and domains was killed off, leaving module that did this stuff stuck. Can we at least wait until async hooks are not experimental before adding in a bunch of internal code depending on them?

[dougwilson]

Based on your example, it seems we can support async hooks while it is experimental without any internal modifications using a middleware wrapper to try it out while Node.js figures out the API, which also decouples the lifecycle of async hooks support from the core framework.

[dougwilson]

function asyncWrap (mw) {
  return function (req, res, next) {
    ​var​ ​currentStore​ ​=​ ​opts​.​asyncLocalStorage​ ​&&​ ​opts​.​asyncLocalStorage​.​getStore​(​)​
    ​if​ ​(​currentStore​)​ ​{​
      ​mw(​req, res, (​err​​)​ ​=>​ ​opts​.​asyncLocalStorage​.​run​(​currentStore​,​ next, err​)​)​
    } else {
      mw(req, res, next​)​
    ​}
  }
}

[dougwilson]

Sorry, I didn't see your first example Wes, that is basically I think what I said too, lol (sorry on mobile)

[wesleytodd]

The main issue is this is exactly the same issue domains had and domains was killed off

FWIW, I strongly don't see that happening here. Also, even if they were killed off in the long run I think (like domains) it would be years. In this regard I also think this is a much more stable solution than domains really ever were.

that is basically I think what I said too

It is basically what you said, but I do think that there is other value in baking it in. If we start with folks using this kind of wrapper that is fine, but in the long run I think this specific feature is so valuable to end users that making it really easy will be best.

[dougwilson]

Right. I think we should consider baking it in. Similar to promises but a faster timeline. If third parties have not stepped in already to provide a wrapping lib, we should so. Then we can work out the kinks both on our side and Node.js side while watching adoption and then graduate it to built in.

Another benefit is that if we only go built in first, folks will have a disconnected experience with favorite middlewares maybe or maybe not making that move yet, so this would seemingly give them a path too I think.

Anyway, I'm just throwing out some thoughts currently. I am not strongly aligned to any of them.

[OllieNilsen]

Thanks guys. This is really helpful. Provides a clear and simple way to handle other async middleware, as well.

[vdeturckheim]

I recall fastify to have a simmilar body-related issue that got fixed by using AsyncResource, https://github.com/fastify/fastify-request-context/pull/12 should help here :)

This should do the trick https://github.com/fastify/fastify-request-context/pull/12/files/d859cfa26f55763efecfb201dfb10f0ba4999dbb#diff-5144edf01860d9bc9d5efb3df22d8765R15-R26