async-listener
async-listener copied to clipboard
First steps of adding async hooks
Hey, I started a tentative of adding async hooks here. I still have a few issues to fix, but I wanted to check if:
- I am on the right track
- this PR is likely to be merged
Wdyt?
I personally think that the purpose of this module is to function as a sort of fallback in case AsyncHooks isn't available, but I think it's better to leave the async hooks implementation up the consumer of this module instead of baking it into the module it self.
But there might be a use-case that I hadn't thought about?
Hey, thanks for the answer, my current use case is: I am using the CLS in Sqreen therfore, in order to update it to use Async Hooks when relevant, I started by building a clone of async-listener beased on Async Hooks (so I don't have to change my code consuming the CLS). The job to do is to build the same api as async-listener with Async Hooks -> It maed sense to me to just have the current module change strategy here.
To wrap up, all I need is the async-listener API based on Async Hooks in order to exploit that in the CLS without changing my code.
WDYT ?
It's not a big maintainability overhead to add async hook support to this module, so as such I'm not against it. But I'd like to hear some thoughts from the other maintainers of this module.
In any case, if we go ahead with this, I think existing index.js file should be renamed patch.js and then a new index.js should be created that either requires async-hooks.js or patch.js depending on the Node.js version. This keeps the two paths separated and clean and also makes the PR diff a little simpler.
I am using the CLS in Sqreen
I think for CLS the better option is to use cls-hooked (or wait until/if it becomes upstreamed). Given how "easy" it is to implement CLS on top of async_hooks directly, putting multiple layers of indirection between CLS and async_hooks seems backwards.
See also https://github.com/othiym23/node-continuation-local-storage/issues/118.
CLS is universally just trying to link async barriers to form that context though, so whether it is async_hooks, AsyncListener, or the monkey patches provided in the async-listener abstraction, they all achieve the same goal. I think it makes sense to have one universal module that provides that abstraction over all the underlying tech options to allow the functionality to exist in a performant way across a wider range of Node.js versions.
If there's a desire for a low-level interface/abstraction that can be used by older versions of node, wouldn't it make more sense to create an async_hooks polyfill instead of the other way around? Otherwise I assume that users would be locked out of using a pass-through to the native (supposedly faster/better..?) option medium and long term.
I don't think a complete polyfill of async_hooks is possible. It'd have to be slightly abstract to reduce it to only the parts of async_hooks that are actually reproduce-able in userland, which this module already does.
@Qard explained why I wanted this PR better than I could. ATM, I have reimplemented a clone of the CLS in my agent but it is basically a copy of the sources with a few changes. Maybe a change in the CLS rather than here would make more sense?
Giving CLS-hooked, I'm not keen about having two different CLS at once from external sources.