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

Recent async-hook update breaks trace on node v4 #24

Closed
timkendrick opened this issue Apr 6, 2016 · 4 comments
Closed

Recent async-hook update breaks trace on node v4 #24

timkendrick opened this issue Apr 6, 2016 · 4 comments

Comments

@timkendrick
Copy link

Hi there – thanks for trace, it's saved my neck countless times!

I'm running Node LTS (v4) in production, and a recent update to async-hook has meant that calling require('trace') now breaks my app.

I notice that since v1.3.0, async-hook now only supports Node v5. Seeing as this was only a minor semver bump to the async-hook module (despite being a breaking change), that means that trace will depend on the new version, and therefore implicitly requires node v5.

I imagine I'm not the only one running the LTS version of node – is there a way we can get trace working on v4 as well as v5? Thanks!

@AndreasMadsen
Copy link
Owner

I'm running Node LTS (v4) in production

I can't recommend running trace in production. trace causes serious performance penalties, because it needs to capture the stack trace for every async operation you perform.

I notice that since v1.3.0, async-hook now only supports Node v5. Seeing as this was only a minor semver bump to the async-hook module (despite being a breaking change), that means that trace will depend on the new version, and therefore implicitly requires node v5.

You are correct. However async-hook uses async_wrap from node.js which is an undocumented API that we are working on. Thus there will be API changes in patch-updates in node.js. This makes it practically impossible to conform with semver standards.

Thus async-hook uses the following approximation:

  • patch: non breaking fix
  • minor: update to a new async_wrap API from node.js
  • major: API addition or API breaking on the async-hooks end

Is there a way we can get trace working on v4 as well as v5? Thanks!

The above rules means that you can use async-hook and maintain backward compatibility by installing the latest major stable version ^1.0.0 that supports your current node version (in your case 1.2.0).

Unfortunately npm doesn't check your node version and there is no flag to enable that. Thus the best you can do is to remove the async-hook module manually and install 1.2.0.

I imagine I'm not the only one running the LTS version of node

No you are not. I have been thinking about removing async-hook from the dependencies list and add an install script, that will download the correct async-hook version automatically. But I'm too busy for that right now. PRs are appreciated.

Eventually the async_wrap changes will be backported to node v4 and the latest version of async-hook will then work. You can ask about the progress here: nodejs/Release#86

@timkendrick
Copy link
Author

Hey there – thanks for the thoughtful response, sorry for the delay in replying (and the accidental commit-spam!)

Completely understand all your points, it sounds a nightmare trying to keep in sync with a private API. For me, the additional error information is definitely worth the performance hit, but if it's not for production use then I guess you could put a warning on the readme? (also that it requires node v5?) Up to you of course.

I didn't realize you also made async-hook, impressive work! I'll submit a PR for that package, which reinstates the prior behavior on Node v4 – let me know on that thread if this is a step in the wrong direction.

Thanks!

@AndreasMadsen
Copy link
Owner

For me, the additional error information is definitely worth the performance hit, but if it's not for production use then I guess you could put a warning on the readme?

Yeah, I had such a warning a long time ago, I should properly get that back.

@AndreasMadsen
Copy link
Owner

Node.js v4.5.0 is now available. This has the latest AsyncWrap API and thus trace should work out of the box.

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

No branches or pull requests

2 participants