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

Handling permissions errors on file I/O #64

Closed
khawkins opened this issue May 14, 2013 · 5 comments
Closed

Handling permissions errors on file I/O #64

khawkins opened this issue May 14, 2013 · 5 comments
Labels
bash compat fix

Comments

@khawkins
Copy link

@khawkins khawkins commented May 14, 2013

It would be nice if shelljs gracefully handled file I/O errors due to permissions issues, as opposed to the current full error dump you get now:

    var shelljs = require('./shelljs');
    shelljs.mkdir('-p', '/var/log/myNewDir');

    $ node shelljsConsumer.js
    shell.js: internal error
    Error: EACCES, permission denied '/var/log/myNewDir'
        at Object.fs.mkdirSync (fs.js:642:18)
        at mkdirSyncRecursive (shell.js:1649:8)
        at shell.js:541:7
        at Array.forEach (native)
        at Object._mkdir (shell.js:526:8)
        at Object.mkdir (shell.js:1491:23)
        at Object.<anonymous> (shelljsConsumer.js:2:10)
        at Module._compile (module.js:456:26)
        at Object.Module._extensions..js (module.js:474:10)
        at Module.load (module.js:356:32)

It seems like, for a lot of use cases (like this one), a more user-friendly error handling path could be initiated.

@jeffheifetz
Copy link

@jeffheifetz jeffheifetz commented Sep 16, 2013

It would also be nice if at least shelljs still respected the fatal argument. Since this is an OS error it always calls process.exit (https://github.com/arturadib/shelljs/blob/master/src/common.js#L179)

@ghost
Copy link

@ghost ghost commented Jan 27, 2014

Bumping this issue. I'm trying to use shelljs in a server environment because frankly, it's way easier to use than the built-in nodejs logic and my use is minimal so I'm not too worried about synchronicity performance issues. I've been using exec() to mv files when needed instead of mv() because I can't have shelljs killing my server process just because it encountered an EACCES error. I would actually vote that shelljs never terminate the process and just throw exceptions instead; let the developers handle the errors! Barring that, my second vote would be to at least make shelljs respect the config.fatal setting in all circumstances, as suggested by @jeffheifetz, so the behavior could be overridden when appropriate.

@akras14
Copy link

@akras14 akras14 commented Feb 13, 2015

mv has a similar issue.

var shell = require("shelljs");
shell.mv("test", "/");

$: node test.js 
shell.js: internal error
Error: EACCES, permission denied 'test'
    at Object.fs.renameSync (fs.js:554:18)
    at /home/alexk/projects/support/uploader/node_modules/shelljs/src/mv.js:77:8
    at Array.forEach (native)
    at Object._mv (/home/alexk/projects/support/uploader/node_modules/shelljs/src/mv.js:53:11)
    at Object.mv (/home/alexk/projects/support/uploader/node_modules/shelljs/src/common.js:186:23)
    at Object.<anonymous> (/home/alexk/projects/support/uploader/test.js:3:7)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)

In comparison exec seems to handle it gracefully, so I am switching to exec for my purposes.

var shell = require("shelljs");
shell.exec("mv test /");

$: node test.js 
mv: cannot move ‘test’ to ‘/test’: Permission denied

@nfischer
Copy link
Member

@nfischer nfischer commented Jul 25, 2016

#474 fixed this issue for mkdir(). Would anyone want to open a PR for mv()?

@nfischer
Copy link
Member

@nfischer nfischer commented Jul 12, 2018

This exception just means "there's a bug in shelljs, it didn't handle something it should have handled." So, this issue is about a very general problem.

I already fixed the mkdir issue in #474, and I filed an issue specific to mv in #872. I'm closing this in favor of the mv-specific issue.


Anyone stumbling across this thread: if you have a similar exception, please file a new github issue, as it will likely require a unique patch.

@nfischer nfischer added fix bash compat labels Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bash compat fix
Projects
None yet
Development

No branches or pull requests

4 participants