gulpjs / fs-mkdirp-stream

Ensure directories exist before writing to them.
MIT License
8 stars 7 forks source link

System Errors don't seem to have a stack trace #3

Open phated opened 6 years ago

phated commented 6 years ago

In exploring #2 and gulpjs/async-done#45, I found that Errors generated by syscalls (like ENOENT from a fs.stat) don't seem to have a Stack Trace. You can get them to have one by calling Error.captureStackTrace(err) but we don't do that on any of our errors that come from node's fs module.

@erikkemperman @contra @terinjokes do ya'll think we should capture stack traces before forwarding the errors? It's pretty annoying to track down Error: ENOENT: no such file or directory, stat '/Users/phated/test/testcase-vinyl-bug/outbad'

erikkemperman commented 6 years ago

Traces are good. But it looks like it has to be done for every error instance, so in a lot of places. Would it make sense to try and bolt this onto graceful-fs, behind an option, since we are using that instead of vanilla fs pretty consistently across all modules?

phated commented 6 years ago

Not sure that isaacs will accept something like that to graceful-fs but maybe we can try?

erikkemperman commented 6 years ago

Oh! Didn’t know that was his. Yeah, no, I’m not holding my breath...

yocontra commented 6 years ago

@phated graceful-fs is less of a hairball than node-glob so I would be in favor of forking it if the maintainers aren't being responsive.

We could merge any of these in that seem useful as well: https://github.com/isaacs/node-graceful-fs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc and finally fix a bunch of our longstanding bugs with that project.

Eventually it might be a good idea to fork node-glob as well and finally have independence to solve our own problems 100%.

phated commented 6 years ago

I've already had to take on a lot of dependencies to make sure everything was stable back to node 0.10 - I don't want to take on an FS module also 😞

phated commented 6 years ago

There's a chance that graceful-fs is losing some error improvements from newer node versions. 😭

BridgeAR commented 6 years ago

Heads up: In Node.js there is currently work in progress to improve that and in new versions the errors will also contain stack traces.

See https://github.com/nodejs/node/issues/18106 as tracker. Some errors are already getting improved right now: https://github.com/nodejs/node/pull/17914 and https://github.com/nodejs/node/pull/18348

coreyfarrell commented 4 years ago

I just ran a test for this, I created issue-3.js:

'use strict';

var fs = require('fs');
var path = require('path');

var outputDirpath = path.resolve('link');

try {
  fs.symlinkSync('not-exist', outputDirpath);
} catch (error) {
  // This shows an EEXIST stack trace on second run
  console.log(error);
}

fs.mkdir(outputDirpath, function(err) {
  // No stack trace here
  console.log(err);
});

fs.stat(outputDirpath, function(err) {
  // No stack trace here
  console.log(err);
});

The above script shows that the lack of stack trace is not caused by graceful-fs or fs-mkdirp-stream. Verified the issue on latest releases of node.js 4/6/8/10/12/14. Using graceful-fs instead of fs in the script yields the exact same results.

arogg commented 4 years ago

My solution gives very nice stacktraces.

Can someone tell me WHY this works? (because it does) (fs is fs.promises for me BTW)

for(const name in fs){
    const func = fs[name];
    if (typeof func==='function'){
        fs[name]=async (...args)=>{
            const mystack = new Error().stack;
            try{
                return await func(...args);
            }catch(e){
                e.message+=mystack;
                throw e;
            }
        }
    }
}