Node.js server integration - isolated context per request

[shalvah]

Like #688, but for traditional single-threaded, server-side Node.js apps. The goal is to ensure that context does not "spill over" across concurrent requests being handled by the same process.

Problem

First, a simple proof of concept (tested with the included express example):

let i = 0;

app.get("/", (req, res) => {
  let reqId = ++i;
  console.log(`Starting request number ${reqId}, current context: ${JSON.stringify(Honeybadger.__context)}`);
  Honeybadger.setContext({ reqId });
  setTimeout(() => {
    console.log(`Ending request number ${reqId}, current context: ${JSON.stringify(Honeybadger.__context)}`);
    res.send("Done")
  }, 1000);
});

Then make two concurrent requests (I use autocannon):

autocannon --connections 2 --amount 2 http://localhost:3000

Terminal logs:

Example app listening at http://localhost:3000
Starting request number 1, current context: {"user_id":"1"}
Starting request number 2, current context: {"user_id":"1","reqId":1}
Ending request number 1, current context: {"user_id":"1","reqId":2}
Ending request number 2, current context: {"user_id":"1","reqId":2}

You can see the problem—the context is shared globally.

Fix

AsyncLocalStorage

Thankfully, @subzero10 has already implemented the stores in his recent PR to start this off.

To Do:

• Add a test that shows this
• Verify Node.js versions supported for this. If we're still tied to older versions, we may be able to use a polyfill like ALS' predecessor, continuation-local storage (although it doesn't seem to still be maintained), or just leave it as-is.

cc @subzero10 @joshuap

[subzero10]

Hey @shalvah, thank you for submitting this!

I already have some thoughts on how to implement this; I will put them in writing here (in a day or two) and we can discuss.
Have you thought on how this would be implemented?

[shalvah]

Thoughts:

Manual wrapping

Wrap the request handler in als.run(). But this will require the users to wrap each of their request handlers, and I don't know if we want to add that friction.

Middleware

We could use an Express middleware, that does something like als.run(store, () => next(req, res)).

This might be good for Express users, but we need to be intentional about error handling, since als.run() returns a promise, and Express v4 does not natively handle promise handlers correctly.

If we go with this, we'd then have to provide similar middleware for other frameworks, or fall back to the wrapping method for unsupported frameworks.

Local instantiation

There might also be a non-ALS option: tell users to instantiate the Honeybadger client on each request. A middleware like:

app.use((req, res, next, err) => {
  req.hb = Honeybadger.(...);
});

But:

1. I'm not sure this would work. I think the package expects to be used in singleton mode🤔
2. This would be cumbersome when calling Honeybadger further down in your stack. You'd have to pass req.hb all the way down to wherever you want to use it.

Monkey-patching

This might provide the best UX, but involved the worst code. We could hook into Express and overwrite its request handling so that it internally runs each request in a local HB context. I believe some APM libraries do something similar.

Pros: The user would likely not need to make any code changes.
Cons: We're essentially forking Express on their machine without their knowledge, and we might inadvertently break other code that depends on it. If a problem occurs, debugging for the end user would be difficult.

Final thoughts

I haven't really thought this through wholly, but I suspect we might be able to use async_hooks and drop deeper than ALS for an alternative approach.)

[shalvah]

@subzero10 are you already on this, or can I pick it up? I've got an app I'd like to track with Honeybadger, but the "leaking context" is a pain to work around.

[subzero10]

No I'm not, you can go ahead!

[shalvah]

@joshuap what do you think of these approaches?

[subzero10]

@shalvah How about we do the middleware solution for frameworks that we already provide middleware (Express & Connect)?

[shalvah]

Right. And for non-supported frameworks, maybe we provide a hb.run(handler) which is a wrapper over als.run(getContext(), handler)? 🤔

[subzero10]

Hmm, I'm not sure, I haven't seen such feature in other tools. I think I need to think/research about this more.

[shalvah]

Elastic APM uses a mix: they patch the http module to automatically start a transaction, but the internal transaction context seems to use the approach I mentioned last (custom async hooks).

Sentry...seems to also have the same issue we do. Example report, tracking issue. Welp.

[shalvah]

I think I'll go with the custom middleware/hb.run(...) approach. I like that monkey-patching needs no end user code changes, but it can go hairy quickly.

• Monkey-patching takes a lot of care to implement thoroughly. I've worked with Express' internal API, and it's a mess.
• It's fragile. We'd need to rework our patch if a new version of the framework changes things.
• Currently isn't fully supported on ESM (do we support ESM?)

I generally try to do less monkey-patching these days, for reasons like these.

The middleware/wrapper approach would gracefully support Express and other frameworks with a simple code addition.

[shalvah]

Got a proof of concept working:

Repurposed the existing requestHandler middleware (which doesn't do anything, but I'll explain in the PR), so no user code changes required.

[joshuap]

@joshuap what do you think of these approaches?

I like the middleware/run approach. We can always go deeper in the future, but I agree with what you said about monkeypatching.