search

usdigitalresponse / arpa-reporter

Web app to aid government agencies in reporting on ARPA grants.
Apache License 2.0
3 stars 0 forks source link

Adopt AsyncLocalStorage to manage tenant_id #473

Closed ajhyndman closed 2 years ago

ajhyndman commented 2 years ago

The AsyncLocalStorage API was introduced in node 14 to provide an async-friendly alternative to thread-local storage in other programming environments.

In Express, this is particularly useful for capturing request-specific context in such a way that they can be shared across multiple asynchronous function calls without interfering with concurrent requests.

It looks like AsyncLocalStorage was promoted to stable as of node 16.4.0.

https://nodejs.org/api/async_context.html#asynchronous-context-tracking

Given the confidence that this API probably isn't going away, I decided it was worth trying out at least!

Happily, the API seems to do what it claims. I didn't run into any issues working with tenant IDs, and the code required to get it working is all of about six lines!

Open questions:

  1. Is request the right object to expose via AsyncLocalStorage?

I considered wrapping the express request in some kind of context object (e.g. Context: { req, session, tenant_id }). I think it may make sense to do that at some stage, but I prefer starting with the simplest possible solution that works, until we know better.

  1. Should we try to manage transactions using AsyncLocalStorage as well?

This one I'm very open to input on. I think it's not quite clear we want to always assume a single global transaction, correct?

ajhyndman commented 2 years ago

Okay, I poked at it some more and did find one sticky issue.

Happily, the JSON parser middleware was just recently updated to ensure compatibility with AsyncLocalStorage. However, the upload middleware that we are using, multer, has not yet been fixed.

There's an open issue and PR to resolve it, but it wasn't too hard to fix in user space, once I understood the issue.

https://github.com/expressjs/multer/issues/814

ajhyndman commented 2 years ago

cc @mattbroussard

ajhyndman commented 2 years ago

In practice the request is the context object with the way req.session is used now (and the way plugins like multer add stuff onto req).

Yeah, for better or for worse this pattern seems pretty well-established today.