Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First steps of adding async hooks #135

Closed
wants to merge 1 commit into from

Conversation

vdeturckheim
Copy link
Contributor

@vdeturckheim vdeturckheim commented Jan 2, 2018

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?

@watson
Copy link
Collaborator

watson commented Jan 3, 2018

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?

@vdeturckheim
Copy link
Contributor Author

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 ?

@watson
Copy link
Collaborator

watson commented Jan 3, 2018

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.

@jkrems
Copy link
Contributor

jkrems commented Jan 3, 2018

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 othiym23/node-continuation-local-storage#118.

@Qard
Copy link
Collaborator

Qard commented Jan 11, 2018

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.

@jkrems
Copy link
Contributor

jkrems commented Jan 11, 2018

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.

@Qard
Copy link
Collaborator

Qard commented Jan 11, 2018

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.

@vdeturckheim
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants