[async hooks] proposal for standard CLS API - request for feedback

[puzpuzpuz]

Hi guys,

I believe that upcoming executionAsyncResource() function (nodejs/node#30959) will allow building a simple and robust CLS API as a part of async_hooks. I've created an experimental PR that shows how this API could look like: nodejs/node#31016

Early reviews and feedback from you would be very valuable.

[Qard]

There's also nodejs/node#26540, which is currently under active development too. It's worth comparing the two to consider the flexibility and performance of each. It's a fairly performance-sensitive area, and is prone to memory issues. We should consider carefully what our needs are from this API.

[puzpuzpuz]

@Qard
I'm aware of nodejs/node#26540 (and of nodejs/node#27172). I have described design ideas in that PR, but as nobody was interested I decided to implement them myself.

The description section in my PR has an complete overview, but let me compare my implementation with nodejs/node#26540 here.

Pros of proposed API:

• Safer memory-wise:
  • Does not depend on destroy hook, thus doesn't lead to mem leak in misbehaving AsyncResource scenarios.
  • Allows complete disposal of the underlying hook callback and all values by calling .remove() method. Thus, you don't have to restart your node process to remove CLS or replace it with another one.
• Should show at least same performance.
  • Deals with less data structures internally and does less operations in overall.
  • If you're interested in benchmark comparison, I'm willing to write a benchmark and compare both implementations. Just point me at a good example of microbenchmark (or benchmark) that is considered good enough by node core team.
  • Update: see [async hooks] proposal for standard CLS API - request for feedback #345 (comment) for benchmark results.
• Has simpler API, yet provides same features.
  • Does not have .enter() method. API is only 2 mandatory (.get()/.set(value)) and one optional (.remove()) methods.
• Has copy-on-write semantics.
  • I believe that this is a very important property. As a real world example, without this property implementing a request id tracing library based on CLS API (like this one: https://github.com/puzpuzpuz/cls-rtracer) is very tricky.

Cons of proposed API:

• The only disadvantage I know is that .set(value) method has to be called outside of application bootstrap (that's inherited from executionAsyncResource()). But it's not critical at all for a CLS API.
  • Update: looks like this restriction will be removed. See this commit: nodejs/node@793282f

Of course, I'm prepared for my PR to be totally ignored, as it was with nodejs/node#27172.

[Flarna]

As already mentioned in nodejs/node#26540 the async enter() is a blocker for using it in in my case. Also the missing propagation for async functions is a problem.
As nodejs/node#26540 tends to use propagation using the trigger path it would require a triggeringAsyncResource() api to avoid the complex implementation using weakrefs,...

What I miss in general in both CLS APIs is the ability to stop propagation or avoid it for some case. e.g. it's formal correct that a context is propagated forever in case of e.g. a setInterval but practically this is a leak and not what most users want.

Besides that I miss functionality to allow to trigger other side effects on context change. See for example how domains use async hooks - in principle it's a CLS but they have the need to do something on context change. So both proposed CLS APIs are able to replace the use of async hooks in domains.

[puzpuzpuz]

@Flarna

What I miss in general in both CLS APIs is the ability to stop propagation or avoid it for some case. e.g. it's formal correct that a context is propagated forever in case of e.g. a setInterval but practically this is a leak and not what most users want.

In case of setInterval, the .remove() method will help as it stops propagation and removes all stored values in the AsyncLocal. It's not the same as stopping propagation of a particular async call chain branch, but nothing prevents for such API to be added. Say, a separate WeakSet could be introduce. It would store all resources for which the propagation must stop.

Besides that I miss functionality to allow to trigger other side effects on context change. See for example how domains use async hooks - in principle it's a CLS but they have the need to do something on context change. So both proposed CLS APIs are able to replace the use of async hooks in domains.

That sounds like an advanced API to me, which is not required in my use cases. But if there will be multiple votes to include it into the API, I could implement it in a similar manner as in your implementation.

[Flarna]

In case of setInterval, the .remove() method will help as it stops propagation and removes all stored values in the AsyncLocal.

This requires an extra, manual call at all these locations which is exactly the opposite of a "just works" CLS as you have to clutter your application code with hints towards CLS instead of one time configuration.

[puzpuzpuz]

This requires an extra, manual call at all these locations which is exactly the opposite of a "just works" CLS as you have to clutter your application code with hints towards CLS instead of one time configuration.

I can hardly believe in existence of "just works" stop-context-propagation configuration API. Exact logic heavily depends on the application needs.

But I believe that having a way to completely stop the propagation and free the memory is a must for a CLS API.

[puzpuzpuz]

I have modified async-resource-vs-destroy.js benchmark and compared proposed implementation with #26540. Here is the result which I got on my machine:

$ ./node benchmark/async_hooks/async-resource-vs-destroy.js benchmarker=autocannon
async_hooks/async-resource-vs-destroy.js n=1000000 method="callbacks" type="async-local" benchmarker="autocannon": 20,277.6
async_hooks/async-resource-vs-destroy.js n=1000000 method="async" type="async-local" benchmarker="autocannon": 15,877.6
async_hooks/async-resource-vs-destroy.js n=1000000 method="callbacks" type="async-context" benchmarker="autocannon": 16,922.41
async_hooks/async-resource-vs-destroy.js n=1000000 method="async" type="async-context" benchmarker="autocannon": 11,841.2
async_hooks/async-resource-vs-destroy.js n=1000000 method="callbacks" type="async-resource" benchmarker="autocannon": 23,582.4
async_hooks/async-resource-vs-destroy.js n=1000000 method="async" type="async-resource" benchmarker="autocannon": 18,407.2

As expected, AsyncLocal is significantly faster than AsyncContext, but slower than the implementation that stores values as resource object properties.

[vdeturckheim]

@puzpuzpuz is the call to destroy the main point of slowlyness in AsyncContext? As mentionned in the PR, it aims at being updated when @Qard 's PR is merged to not use it anymore.

[puzpuzpuz]

@puzpuzpuz is the call to destroyz the main point of slowlyness in AsyncContext`? As mentionned in the PR, it aims at being updated when @Qard 's PR is merged to not use it anymore.

Probably, but it's hard to tell for sure. Their public APIs are quite different and AsyncContext seems to be using more data structures and doing more stuff under the hood.

[vdeturckheim]

@puzpuzpuz actually this whole part would go away eventuallyhttps://github.com/nodejs/node/pull/26540/files#diff-0bb01a51b135a5f68d93540808bac801R201-R214
https://github.com/nodejs/node/pull/26540/files#diff-0bb01a51b135a5f68d93540808bac801R227-R229

In the end, the only data structure that would remain would be the map used for holding context. I would like to keep it as it is closer to the original CLS implementation that most APM vendors have copied over the years.

I am pretty opinionated on the instanciation of only one Async Hook for the whole process as I find it faster than having multiple ones.

[puzpuzpuz]

@vdeturckheim

@puzpuzpuz actually this whole part would go away eventuallyhttps://github.com/nodejs/node/pull/26540/files#diff-0bb01a51b135a5f68d93540808bac801R201-R214
https://github.com/nodejs/node/pull/26540/files#diff-0bb01a51b135a5f68d93540808bac801R227-R229

In the end, the only data structure that would remain would be the map used for holding context.

Yes, it will remove one Map internally and only one WeakMap + one Map + one Set will remain.

However, this change won't add copy-on-write semantics for AsyncContext. Do you happen to know how to implement a request id tracing (which is one of the most popular use cases for CLS APIs) once AsyncContext gets that change?

With AsyncLocal it's as simple as the following example: https://github.com/nodejs/node/pull/31016/files#diff-9a4649f1c3f167b0da2c95fc38953a1fR601

I would like to keep it as it is closer to the original CLS implementation that most APM vendors have copied over the years.

Nothing prevents APM vendors or 3rd-party library authors to build a thin-wrapper that provide a Map-like on top of an AsyncLocal. Such implementations will be simple and straightforward.

On the other hand, users who want to store a single value in CLS API won't have to pay for an additional Map.

I am pretty opinionated on the instanciation of only one Async Hook for the whole process as I find it fater than having multiple ones.

AsyncContext's init hook has a for loop, so it's still O(N), just as with N different AsyncLocals. AsyncLocal's init hook can be made global without any changes in the public API. But most of real world applications have a single AsyncLocal per the whole node process created by an APM agent or a request tracing library. So, keeping AsyncLocals isolated from each other seem to be more valuable to me.

[vdeturckheim]

Yes, it will remove one Map internally and only one WeakMap + one Map + one Set will remain.

The WeakMap is already gone ;)

Do you happen to know how to implement a request id tracing (which is one of the most popular use cases for CLS APIs) once AsyncContext gets that change?

I am not sure I get the quesiton, I would simply do a run and add the id in the map under a named key

Nothing prevents APM vendors or 3rd-party library authors to build a thin-wrapper that provide a Map-like on top of an AsyncLocal. Such implementations will be simple and straightforward.

Fair point, but a few comments on the PR actually asked how close it is to current existing implementation. IMO this is a design goal to rely on something that is already used.

On the other hand, users who want to store a single value in CLS API won't have to pay for an additional Map.

I don't think this Map is expensive here.

AsyncContext's init hook has a for loop, so it's still O(N), just as with N different AsyncLocals. AsyncLocal's init hook can be made global without any changes in the public API. But most of real world applications have a single AsyncLocal per the whole node process created by an APM agent or a request tracing library. So, keeping AsyncLocals isolated from each other seem to be more valuable to me.

TBH, this choice is mostly based on nodejs/node#16222 (comment).

but this is probably more discussions to have in the respective PRs.

[puzpuzpuz]

@vdeturckheim

The WeakMap is already gone ;)

Oh, I've missed that change.

So, it puts the Map into resources directly now. In that case, it won't be possible to get rid of AsyncContext completely (a la AsyncLocal.remove), except for restarting the node process.

I am not sure I get the quesiton, I would simply do a run and add the id in the map under a named key

A AsyncContext.run per each HTTP request then? Thus, plus one Map and one process.nextTick per each request. Doesn't sound like something that is good for performance.

Fair point, but a few comments on the PR actually asked how close it is to current existing implementation. IMO this is a design goal to rely on something that is already used.

As far as I understand, main goal for core APIs is to provide flexible (and sometimes low-level) APIs that can be used directly or used to build user-land libraries on top of them. But maybe this time it's an exception to the rule.

I don't think this Map is expensive here.

I believe that for core APIs, every data structure counts.

TBH, this choice is mostly based on nodejs/node#16222 (comment).

but this is probably more discussions to have in the respective PRs.

Not sure if that comment is applicable to CLS API. Anyway, it can be easily changed in both directions without any changes in the external behavior.

[Flarna]

I would like to keep it as it is closer to the original CLS implementation that most APM vendors have copied over the years.

@vdeturckheim May I ask which APM vendor implemenations you refer to?

My search shows different implemenations:
New Relic: In before hook they set their context and in the after hook they restore the old one.

DataDog: In before hook they set their context by a sync enter call and in after hook the clear it.

Elastic: This is different, they do a lookup based on executionAsyncId() in their map.

OpenTelemetry: There a with() function is used to execut a function synchron after setting the context taken from a map using executionAsyncId() and restore the previous value

I'm working at dynatrace (unfortunatelly our agent is not open source so no link 😢) and we use mostly the same mechanism as NewRelic/DataDog.

There is cls-hooked which has an API which reminds me to AsyncStorage. I think this lib is usually used by application programmers but not APMs. APMs just share the idea and the mapping to async_hooks.

---

As there was some link above towards the ticket to use async_hooks in domains. Would it be possible to port Domains to use the proposed CLS system to have a proove of concept?