AndreasMadsen / trace

Creates super long stack traces
https://trace.js.org
MIT License
194 stars 18 forks source link

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

Closed timkendrick closed 8 years ago

timkendrick commented 8 years ago

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

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:

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: https://github.com/nodejs/LTS/issues/86

timkendrick commented 8 years ago

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

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

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