AsyncResource constructor arguments

[jasnell]

Currently, the AsyncResource constructor requires a type argument, e.g. new AsyncResource('foo'). It will throw if this is not provided. This information, however, is only used for async_hooks integration and is not necessary for async context propagation. I'd like to propose that we make the argument optional, defaulting its value to this.constructor?.name || 'AsyncResource' when it is not provided.

/cc @Qard @legendecas @nodejs/async_hooks

[Qard]

Seems reasonable to me. Possibly related though, and we've already discussed a bit privately, is the idea of AsyncLocalStorage having its own binding mechanism. It could be layered on top of AsyncResource and call it whatever it wants because AsyncLocalStorage doesn't actually care about resource names so that would be another solution, albeit one that would involve more ecosystem migration effort. I don't see why both can't be done though.

[jasnell]

I think that makes sense but I'd hesitate to have to create new API surface at this point with AsyncContext coming down the road.

[Flarna]

As long as async_hooks is still a thing and has use cases no covered by AsyncLocalStore it's not that nice to create potentially a lot different resource types same name. I know this doesn't force anyone to do so but you know programmers are lazy and if there is nothing provides nothing is given.

Some people use async_hooks for resource tracking and it's valuable to have a good name there. As these tracking tools and the creators of AsyncResources are likely different persons one doesn't see the need of the other.

If someone creates a derived class it's just a matter to pass it to super at a single place.

Even if we create a replacement API for resource tracking I would assume the resource type is valueable info there.

[jasnell]

I agree that the value is useful for async_hooks cases, which is why I'd like to see it use a reasonable default value. The situation is not unlike anonymous functions. If someone really wants to be able to differentiate the resources, then they should specify a usefully distinguishable type, otherwise it falls back to a generic default.

[Flarna]

Frequently the consumer of this info (APM tool, logger, crash analyser,...) is not the same person as the creator of the AsyncResource (DB driver,...).

As a result the consumer gets bad data and the final user of this tool complains that it is not helpful.

sure, a PR on the producer repo is always possible but why should we go this rocky path just because of a single string given to a constructor?

[jasnell]

why should we go this rocky path just because of a single string given to a constructor?

Largely because it simply adds friction for other runtime implementations that want to implement Async context tracking in a portable way but have no intention of implementing async hooks in general. The name is generally not useful for those cases an requiring that it be there and be a non-empty string is a nuisance (admittedly a small one)

[Qard]

I think that makes sense but I'd hesitate to have to create new API surface at this point with AsyncContext coming down the road.

AsyncContext includes a wrap function which is essentially the same as what I was thinking for the bind. Though I was also thinking of having a per-store bind which would be an addition.

[Flarna]

Keeping the name in other runtimes allows to add a resource tracking later.

[Qard]

I don't think resource tracking should ever be a concern of AsyncLocalStorage or AsyncContext. They are two different concepts and never should have been mashed together like they were with async_hooks. A proper resource tracking API should exist separately.

[jasnell]

As far as workerd/cloudflare workers is concerned, we never intend to implement async_hooks or a similar resource tracking model and therefore would never have use for the type.

[Flarna]

I don't think resource tracking should ever be a concern of AsyncLocalStorage or AsyncContext. They are two different concepts and never should have been mashed together like they were with async_hooks. A proper resource tracking API should exist separately.

Fully agree but this issue is about AsyncResource.

[jasnell]

Fully agree but this issue is about AsyncResource.

AsyncResource straddles a line between resource tracking and async context propagation so @Qard's point here is valid. This class tries to serve both purposes which works great for node.js but the resource tracking requirements are a bit clumsy in other environments.

[Flarna]

So you propose to put away the Resource part of AsyncResource?

e.g. add (or rename to) AsyncOperation or AsyncContinuation class which participates in context propagation but not in resource monitoring?

[jasnell]

No, we don't want to rename the class. I simply want to default the type to something reasonable if it is not provided explicitly. The change would not impact existing code since existing code would always be passing a name so it would not be a breaking change.

[Flarna]

I didn't meant to rename. Add AsyncOperation or AsyncContinuation which requires no resource type and participates only in context propagation and put AsyncResource on top of it which includes it in resource tracking.

[jasnell]

As I've said, I do not really want to introduce new API surface while AsyncContext is pending. It's likely to introduce its own constructs here and anything new we add here possibly causes confusion down the road. A subset of AsyncResource works for what we need.

@Qard has mentioned a few times about adding a static AsyncLocalStorage.bind(fn) that would essentially be an alias to AsyncResource.bind(...) and I think that would be useful but doesn't fully account for the class Foo extends AsyncResource case (for example, see EventEmitterAsyncResource).

[Flarna]

Agree.
Not sure if modeling EventEmitterAsyncResource as an AsyncResource was the best choice as it's more a bind helper and doesn't do anything async.
But as usual it's easy to complain in retrospect - that time I approved :o) - and it's too late to change this.