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

Use AsyncHook API #4

Closed
overlookmotel opened this issue Jun 4, 2017 · 9 comments
Closed

Use AsyncHook API #4

overlookmotel opened this issue Jun 4, 2017 · 9 comments
Assignees

Comments

@overlookmotel
Copy link

Hi.

Are you planning to convert this module to use the new AsyncHook API in Node 8? If so, any guess as to when that might happen?

Many thanks!

@Jeff-Lewis
Copy link
Owner

Yes, I'm working on the migration now.

It looks like @AndreasMadsen cleaned out his async-hook wrapper and it just returns async_hooks from node-core now...

Anyone have any opinions on how we want handle existing older node versions v4-v7?

If node < 8 then require('async-hook') (as it does today)
else node = 8 then require('async_hooks') ?

The API's are not 100% matching so we would need to have multiple code paths for handling node 4,6,7 and node 8.

I assume this will be a semver major so I'm planning this to be v5 of cls-hooked.

Let me know if anyone wants to help. I'll have a new branch pushed shortly.

@AndreasMadsen
Copy link

It is not just the API that is different there is also a lot of behavioral changes. If there where behavioral changes I might have redone the API for backward compatibility, but with the behavioral changes it would close to impossible.

You should definitely upgrade, there should also be a lot of performance gain. The old version of async-hook will continue to exist in npm if you would like backward compatibility.

@Jeff-Lewis
Copy link
Owner

FYI - A new branch of cls-hooked using async_hooks is in progress but still failing and not complete. Some bugs are being worked out in node with regards to async_hooks and Promises. See node 8.1.1 proposal and list of async_hooks's PRs.

@Jeff-Lewis Jeff-Lewis self-assigned this Jun 11, 2017
@holm
Copy link

holm commented Jun 12, 2017

Thanks for the update. cls-hooked with async_hooks is one of the things I look forward to the most in Node 8. Thanks for the all the effort.

@Conduitry
Copy link

Just sharing this here in case someone finds it interesting:

The past day or so I've been reading up on async_hooks and working on my own tiny library that does a similar sort of thing as it sounds like this branch is going to. Here are the library and the original Gist which better conveys the central idea.) It's definitely still a toy but I've used it successfully in a couple things.

It currently requires a nightly version of Node - the necessary features and bugfixes of which I think will be available in 8.2.0. (In Node 8.1.2, I couldn't get awaiting on promises to preserve the async chain.) I intend to follow the async_hooks changes that come out in the next few Node versions and see when this stuff works for real, or whether I need to make any changes.

I'm not sure of the full scope of the features of continuation-local-storage/cls-hooked, but I was surprised how little code was needed to reach the core goal.

@Jeff-Lewis
Copy link
Owner

@Conduitry very cool. It's nice to see an ES6 version. Taking a quick look, some differences came to mind. There might be more which I'll find after a spend a little time on it.

  • Context Inheritance: In cls-hooked contexts inherit their parent context's properties to carry down the execution chain.
  • Namespaces: group contexts by namespace
  • Support manually entering and leaving a context: cls-hooked allows consumer to call namespace.enter(ctx1) and namespace.exit(ctx2) and tracks internally the start and end of each context, as does node-continuation-local-storage. I think the main use case for this was for monkey-patching other libs to work with cls. That said, with the new async_hooks ids, we might not need to track it anymore. I'll test this when the promise bugs are fixed in hopefully node v8.2.

I think I'll use your async/await/sleep/promise/timeout example and make a test out of it. :)

@Conduitry
Copy link

Yep your first two points are things that I think my implementation also handles. I did take a peek at the features offered by continuation-local-storage while writing my library. :)

New contexts inherit the parent context (using Object.create of the existing context if present, rather than of null).

Namespaces are handle by just having different instances of the main class. The namespaces don't need to have unique names - but they do keep their own context objects separate.

And yeah I think that third point you mentioned is something that's hopefully unnecessary with async_hooks.

@Jeff-Lewis
Copy link
Owner

@Conduitry - Check to make sure your lib works with non-Promise contexts. cls-hooked uses triggerAsyncId for Promises and executionAsyncId for others.

Resolved in cls-hooked v4.2.0

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

No branches or pull requests

5 participants