Jeff-Lewis / cls-hooked

cls-hooked : CLS using AsynWrap or async_hooks instead of async-listener for node 4.7+
BSD 2-Clause "Simplified" License
760 stars 89 forks source link

Use AsyncHook API #4

Closed overlookmotel closed 7 years ago

overlookmotel commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.

holm commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

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

Jeff-Lewis commented 7 years ago

Watching:

Conduitry commented 7 years ago

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 commented 7 years ago

@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