Ability for async functions to retreive their CLS context

Is your feature request related to a problem? Please describe.
Please describe the problem you are trying to solve.

Right now, the AsyncLocalStorage object needs to be externally available for the async functions to avail the store.

Describe the solution you'd like
Please describe the desired behavior.

Implement a getContextCLS API that returns the AsyncLocalStorage object under which this asynchronous call was initiated. Return undefined, if it was not run under AsyncLocalStorage semantics.

Illustration:

const { AsyncLocalStorage } = require('async_hooks');

function main() {
  const cls1 = new AsyncLocalStorage();
  const cls2 = new AsyncLocalStorage();
  cls1.run(() => {
    const store = cls1.getStore()
    store.set('x', 'y')
    setTimeout(foo, 1000)
  })
  
  cls2.run(() => {
    const store = cls2.getStore()
    store.set('x', 'z')
    setTimeout(foo, 1000)
  })
}


function foo() {
  // obtain the cls object here that this async call was part of
  // for example:
  // const cls = getContextCLS()
  //  const store = cls.getStore()
  //  console.log(store.get('x')
  // prints 'y' and 'z' as and when those are called.
}

main()

Use case: in a concurrent workload scenario, it is not easy to maintain AsyncLocalStorage objects globally, and across multiple async function families.

Describe alternatives you've considered
Please describe alternative solutions or features you have considered.

Alternative is to share these objects globally (which is not very attractive)

@vdeturckheim

/cc @vdeturckheim

My understanding is that there is no particular AsyncLocalStorage instance associated with any async call, so I’m not sure what getContextCLS() would actually return?

Alternative is to share these objects globally (which is not very attractive)

Why not? I think that’s the thing that conceptually makes the most sense here.

We did not want to make the AsyncLocalStorages globally availabed by default to let users hide their instance and prevent anyone from tempering with it.

As there might be multiple AsyncLocalStorage running on the same code, a method like getContextCLS() would return an array:

const { AsyncLocalStorage } = require('async_hooks');

const asl1 = new AsyncLocalStorage();
const asl2 = new AsyncLocalStorage();

asl1.run(new Map(), () => {
  asl2.run(new Map(), () => {
    AsyncLocalStorage.getActive(); // returns [asl1, asl2];
  });
});

I don't have strong opinions here appart that I'd like to leave the owner of an AsyncLocalStorage the ability to keep it private.

@addaleax

@addaleax - the problem with the global approach is that we need to explicitly maintain the association of AsyncLocalStorage (let us call it ALS) with an async function. For example, in the code in the first comment, foo will be called from both the contexts, in arbitrary order. So even if we make cls1 and cls2 global, how do one decide which ALS to use (to access the store etc.)?

@gireeshpunathil

@gireeshpunathil I think there might be a misunderstanding about how ALS works. In particular, AsyncLocalStorage instances are not async contexts.

So even if we make cls1 and cls2 global, how do one decide which ALS to use (to access the store etc.)?

I don’t quite understand this question… can you maybe give a more concrete use case as an example?

sure:

10 clients connect to a server
those clients share a common non-closure handler function that receive the server data
each client is created within an ALS.run context, so all the callbacks emanating from that context are grouped under one ALS object.
within the handler, I want to accumulate server data into their respective stores.
at the end of each response, retrieve the data from the store, and process

Is it possible to achieve this using ALS APIs?

[ edit: couple of typos]

@gireeshpunathil

@gireeshpunathil in that case you would use one single ASL:

const asl = new AsyncLocalStorage();
server.on('request', (req, res) => {
    asl.run({ req, res}, () => {
       // all calls to asl.getStore() wil return {req, res}
    });
});

Would that make sense in your example?

@vdeturckheim

@vdeturckheim - here is a complete example that exemplifies my use case, but using your suggestion of using a single ALS:

const { AsyncLocalStorage } = require('async_hooks')
const http = require('http')
const cls = new AsyncLocalStorage()
let store
let index = 0

const server = http.createServer((q, r) => {
  r.end((index++).toString().repeat(1024 * 1024 * 10))
})

server.listen(12000, () => {
  cls.run(new Map(), () => {
    for(let i = 0; i < 10; i++) {
      const req = http.get('http://localhost:12000', (res) => {
        const data = ''
        store = cls.getStore()
        store.set('data', data)
        res.on('data', ondata)
        res.on('end', onend)
      })
      req.end()
    }
  })
})

function ondata(d) {
  // keep store globally due to a bug, ref: https://github.com/nodejs/node/issues/32060
  // const store = cls.getStore()
  if (store && store.has('data')) {
    let chunk = store.get('data')
    chunk += d
    store.set('data', chunk)
  } else {
    console.log('error...')
  }
}

function onend() {
  // let store = cls.getStore()
  if (store && store.has('data')) {
  let chunk = store.get('data')
   var re = new RegExp(chunk[0], 'g')
    console.log(`stream type: ${chunk[0]}`)
    console.log(`stream length: ${chunk.replace(re, '').length}`)
  } else {
    console.log('ended, but in error...')
  }
}

the test does these:

sets up a server, that sends large buffers of data, filled with cardinal numbers, increasing per request
sets up a single ALS object, and runs 10 clients inside
sets up an empty store
in the data callbacks, accumulate the instantaneous data with the store data
in the end callbacks, retrieve the store data, and make sure they are homogeneous

the output shows they are not, instead the data is all clobbered between various responses

stream type: 9
stream length: 73947963
stream type: 9
stream length: 73948064
stream type: 9
stream length: 73948165
stream type: 9
stream length: 73948266
stream type: 9
stream length: 73948367
stream type: 9
stream length: 73948468
stream type: 9
stream length: 73948569
stream type: 9
stream length: 73948670
stream type: 9
stream length: 75498381
stream type: 9
stream length: 75498381

expected output:

stream type: 0
stream length: 0
stream type: 1
stream length: 0
stream type: 2
stream length: 0
stream type: 3
stream length: 0
stream type: 4
stream length: 0
stream type: 5
stream length: 0
stream type: 6
stream length: 0
stream type: 7
stream length: 0
stream type: 8
stream length: 0
stream type: 9
stream length: 0

@gireeshpunathil

@gireeshpunathil

  cls.run(new Map(), () => {
    for(let i = 0; i < 10; i++) {

should be

  for(let i = 0; i < 10; i++) {
    cls.run(new Map(), () => {

otherwise all the requests share a single store.

ok, I can test this with #32063 only, because with this change, we will have 10 stores, and with the getStore bug, I will have to manage them separately. Will test and get back, thanks!

@addaleax

@addaleax - with the patch of your PR, and with your suggested patch, the code is working as expected, thanks!

so a single instance of AsyncLocalStorage is capable of handling multiple callback chains (families, groups, etc. whatever we call those), through giving access to the appropriate store when invoked within the asynchronous routines! then why we would multiple instances of AsyncLocalStorage in the first place (why the new operator) ?

that is what confused me.

However, with a single object able to anchor all the stores in an application, keeping it global makes sense to me, and that addresses my use case as well. Not sure if there are other scenarios where this is not the case. Keeping it open for a couple of days to see if any.

thanks once again!

then why we would multiple instances of AsyncLocalStorage in the first place (why the new operator) ?

Because different AsyncLocalStorage instances can be used for different purposes or even from different npm modules, which should not conflict with each other. And in particular, the scopes that they use might not match; for example, in

cls1.run(new Map(), () => {
  cls2.run(new Map(), () => {
    x()
  });
  cls2.run(new Map(), () => {
    y()
  });
});

You’ll see x() and y() sharing a store as far as cls1 is concerned but having different stores as far as cls2 is concerned.

(But I mostly think it’s the fact that we don’t want ALS from different modules to conflict.)

test: add new scenario for async-local storage

@lroal

Wouldn't it be convenient to use a static dictionary object instead of passing around the cls instance to all modules. It would just be helper methods.

@lroal I think something like https://www.npmjs.com/package/@northscaler/continuation-local-storage is what you're looking for. I'll see about enhancing it to also support AsyncLocalStorage.

But I agree that it would be nice for class AsyncLocalStorage to have static methods like create(key) and get(key) that create & get a named AsyncLocalStorage instance, respectively. I'd say the best practice would be to pass a Symbol for the key, but a string would work, too, at the risk of multiple libraries using the same string key.

@matthewadams

@matthewadams thanks. I wrote my own cls library some time ago. node-cls . Though, I was hoping AsyncLocalStorage would replace it entirely. But for me this is not the case yet.
(If you are creating a lib, you can just pass a symbol instead of a string. )

@lroal

@lroal I like your "Await instead of run" feature -- that's a new one on me. I guess in the meantime, I guess we'll just have to Roll Our Own™️ until such time as AsyncLocalStorage supports our features. 😊

@matthewadams

@matthewadams @lroal

I am not sure I get this discussion here.

Getting the let cls = AsyncLocalStorage.create('someKey'); feature seems to me to be as straightforward as an ecosystem module used as a proxy for providing AsyncLocalStorage instances. I still believe having this in core would break the point of allowing people to hide their own instances from the outside (and the Symbol based solution does not really bring anything to the table IMO).

I like your "Await instead of run" feature -- that's a new one on me.

Is there a difference between this and enterWith?

I guess we'll just have to Roll Our Own™️ until such time as AsyncLocalStorage supports our features. 😊

I would be happy to review any PR adding features on this API, also, my opinions can be wrong and an healthy discussion with more collaborators can probably bring ideas here.

@vdeturckheim

@vdeturckheim

I like your "Await instead of run" feature -- that's a new one on me.

Is there a difference between this and enterWith?

It appears not. I wasn't aware of node-cls and I hadn't noticed it on AsyncLocalStorage until I started participating in this thread.

@vdeturckheim

@vdeturckheim

I am not sure I get this discussion here.

The only thing to understand here, IMHO, is that two authors of similar packages (myself and @lroal) utilize a static map of context instances available from anywhere, as opposed to being required to have a reference to a particular instance of one, and that we were surprised not to see something similar in AsyncLocalStorage.

The proposal, at its simplest, would be to add static methods to AsyncLocalStorage that allow for easy creation & retrieval of instances using either namespaced string keys, Symbol keys, or both. I'd expect library authors that leverage ALS to use Symbols and application authors to use Symbols or strings.

I'd be curious to your thoughts, @vdeturckheim, after reviewing node-cls and @northscaler/continuation-local-storage to get a feel of what we expected in AsyncLocalStorage.

I'm totally open to an explanation of how our packages' behaviors are implementable using AsyncLocalStorage natively, rather than leaving it to userland code maintain a static map of instances.

@matthewadams

@matthewadams @vdeturckheim
There is a slight difference between enterWith and start. Start() returns a promise and sets the context on a child resource - not the current resource. EnterWith() will set the context on current resource / asyncId.

@matthewadams

@matthewadams thanks for your response.

As mentionned earlier in the thread, I don't see any gain in such static map. Do you have examples of situations where such a map in core would solve specific problems?

@lroal
you were porbably meaning to ping me in your last message. That's an interesting point. I'd tend to think that doing await context.enterWith() would do the same then. 🤔

@matthewadams

@matthewadams @vdeturckheim
It would make developer life easier. You don't need to pass around/require the reference to the singleton instance everywhere. And if you have a deep file hierarcy, you need to require it by relative path. I suspect application developers will seek for a workaround anyway - either by using some user land module or implementing it themself. Perhaps as a local file package (which has it's problems in combo with npm). So the "workaround" might as well be in core rather than in user land. In my opinion.

@vdeturckheim , about the enterWith() vs start(). I am not sure myself. The case I had in mind was if current "thread" was busy doing something async and start() should not interfere with that context - but rather create it's own child "thread". But, maybe that's purely hypothetical. It is more like synctactical sugar perhaps. 😊

Do you think it is worth creating a PR ? Will it be considered ? It would need tests i guess. I have never create PRs in node before.
Thanks for taking your time.

@lroal

It would make developer life easier. You don't need to pass around/require the reference to the singleton instance everywhere. And if you have a deep file hierarcy, you need to require it by relative path. I suspect application developers will seek for a workaround anyway - either by using some user land module or implementing it themself. Perhaps as a local file package (which has it's problems in combo with npm). So the "workaround" might as well be in core rather than in user land. In my opinion.

Once again, I think the ability to hide an instance from other and that is a must have feature for me (I don't want anyone external to tamper with my instance of AsyncLocalStorage). Be ready for UX discussions :) It will needs the opinion from other collaborators and my concerns can't be the only thing taken in account.

@lroal a PR would, for sure, be considered! There are also considerations about what the UX would look like (create vs get, what about multiple calls?). We are always very excited by any opportunity of welcoming a new contributor to the project!

Housekeeping part, I think this issue has derived a bit from its original point. I will close it, feel free to reopen if needed and let's follow-up either in a dedicated issue or PR.

@vdeturckheim

Thanks @vdeturckheim .
I just want emphasize (as mentioned earlier in this thread), as a library developer you would use a Symbol as key. Then nobody can tamper with your instance of AsyncLocalStorage.

@vdeturckheim

@vdeturckheim @lroal
I forgot to mention a use case that we leverage that uses a static map containing Strings as keys instead of Symbols. We sometimes use JavaScript decorators that dictate that certain contextual information (classically, user & role information) be placed into continuation local storage so that the decorator can access it later to execute logic (again, classically, access control decisions). In this case, Strings work great as keys to the static map of AsyncLocalStorage instances because the decorator library provider intends to use the shared context information to do its job.

FYI, we just released an update to @northscaler/continuation-local-storage that includes support for AsyncLocalStorage in addition to cls-hooked & zone.js! 😊

Navigation Menu

Repository navigation

Ability for async functions to retreive their CLS context #32062

22 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Ability for async functions to retreive their CLS context #32062

Description

Activity

gireeshpunathil commented on Mar 3, 2020

addaleax commented on Mar 3, 2020

vdeturckheim commented on Mar 3, 2020

gireeshpunathil commented on Mar 3, 2020

addaleax commented on Mar 3, 2020

gireeshpunathil commented on Mar 3, 2020

vdeturckheim commented on Mar 3, 2020

gireeshpunathil commented on Mar 3, 2020

addaleax commented on Mar 3, 2020

gireeshpunathil commented on Mar 3, 2020

gireeshpunathil commented on Mar 3, 2020

addaleax commented on Mar 3, 2020

22 remaining items

matthewadams commented on Aug 10, 2020

matthewadams commented on Aug 10, 2020

lroal commented on Aug 11, 2020

matthewadams commented on Aug 11, 2020

vdeturckheim commented on Aug 11, 2020

matthewadams commented on Aug 11, 2020

matthewadams commented on Aug 11, 2020

lroal commented on Aug 12, 2020

vdeturckheim commented on Aug 12, 2020

lroal commented on Aug 13, 2020

vdeturckheim commented on Aug 13, 2020

lroal commented on Aug 13, 2020

matthewadams commented on Aug 16, 2020

matthewadams commented on Aug 16, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions