Async Local Storage for Request Context

[kibertoad]

Is your feature request related to a problem? Please describe.

In order to efficiently isolate identity maps across different requests, it is helpful to use Request Context functionality. However, according to the documentation it is currently implemented using the old Domains API.

Describe the solution you'd like

Node 14 comes with shiny new Async Local Storage: https://www.freecodecamp.org/news/async-local-storage-nodejs/
It could be used as an alternative to Domains API on newer Node versions.

Describe alternatives you've considered

cls-hooked library, but it is barely supported these days.

Additional context

Since Async Local Storage is still an experimental feature, it is essential to cover this functionality with automated tests extensively and be very explicit about which Node versions support this feature. It should probably be an opt-in feature with fallback into old Domains-based implementation.

[B4nan]

Yeah I saw that too, although I already used async_hooks in the early days (early v2). There was huge memory leak, it made the library unusable after some amount of requests. Very hard to find the cause, it just made everything slower gradually... And this leak was present even with barebone express example, without any ORM being involved (I was testing plain express, no addition dependencies or packages used).

So I am not very keen to implement it again, not before they claim it is stable (and still I would be very careful). Domain API on the other hand does not leak at all, it just works.

(also tried cls-hooks, also was leaking hard)

Here is the PR that replaced it with Domain API: #58

Also we are supporting node 8 in v3 and node 10 in v4, so this can't be implemented now, not in this repository.

[kibertoad]

@B4nan In this case "experimental" does not mean "leaks memory", actually implementation took as much time as it did precisely because they were addressing performance and memory leak concerns. They are just leaving the door open to breaking the API without SemVer major.

From my understanding ALS was also backported to Node 12, not sure if there are plans for Node 10. Couldn't 2 different Request Context strategies coexist and be resolved based on Node version, though?

[B4nan]

Well, I know what experimental means. Async hooks are there for quite some time, the new storage helper is just a wrapper on top of it. And async hooks itself were leaking hard, sure they are working on it to not leak :)

They could coexist for sure, but as I said, I already burned myself with this feature once, the storage wrapper itself is the new thing, async hooks were there before.

[kibertoad]

@B4nan I still have the sqlite FK thing to tackle, but after that is behind, would you accept a PR for ALS if I get to trying it out, as an opt-in feature?

[kibertoad]

I think nodejs/node#30959 is how they addressed the memory leak problem.

[B4nan]

Sure, I know that in the long run we will need to use it for sure, so having it there optionally is a good idea, it will also allow easier testing of the leaks (or a confirmation of no leaks :)).

I can imagine some issues with the rest of the codebase being node 10 compatible, but as said, it could have its own repo (or a package as v4 is a monorepo).

Be sure to target dev branch (if #527 wont be merged yet).

[kibertoad]

But if it's conditional based on Node used (or not even that, if it always has to be enabled by user), what kind of issues you are foreseeing? If tests skip for older versions of Node, should just work.

[B4nan]

I would expect TS to be the problem. Tried to use BigInt recently, and although it is present in node 10 lts (which is the minimum for v4), TS requires you to target es2020 to use it (node 12), which I can't do (at least I believe that means that the compiled JS code would not be compatible with node 10).

[kibertoad]

Fair point.

[kibertoad]

I wonder what happens if you target ES2020 but don't use any of ES2020 logic, though. From my understanding it doesn't explicitly break compatibility, just provide no promises on working on anything below that version, but I could be wrong.

[B4nan]

When one targets ES5, it won't use native promises but rather transpile it to generators or what ever it does. So imagine I would want to support ES5 but used target ES6 or above. It would not transpile promises, so the compiled code would break on node 6 or older.

Not sure what exactly is the difference between es2018 and es2020, maybe it would be safe, but this is where my concerns are coming from.

[mikeconley12]

The funny thing is, the domains were re-implemented using async_hooks almost 3 years ago :)
nodejs/node#16222 (comment)

[B4nan]

Instead of porting the RequestContext and making it more complex, I introduced new config option: context: () => EntityManager, with the default value of RequestContext.getEntityManager(). This way it is easy to swap it with AsyncLocalStorage:

const storage = new AsyncLocalStorage<EntityManager>();

const orm = await MikroORM.init({
  context: () => storage.getStore(),
  // ...
});

app.use((req, res, next) => {
  storage.run(orm.em.fork(true, true), next);
});