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

removeSync() does not error if file to remove exists, but is not writable. #902

Closed
rhwood opened this issue May 8, 2021 · 6 comments
Closed

Comments

@rhwood
Copy link

rhwood commented May 8, 2021

  • Operating System:
    • macOS 11.3.1
    • Ubuntu 16.04.6 LTS
  • Node.js version:
    • 16.1.0
    • 16.0.0
    • 14.16.1
  • fs-extra version:
    • 10.0.0

Starting with fs-extra 10.0.0 on node 14.14+, failures to rm a file that exists, but is in an unwritable directory, fail silently. fs-extra 9.1.0 throws an error, and fs-extra on node 12 throw an error.

Note that fs.rm in node 14.14+ is documented to fail silently only if the force parameter is true and the file to delete does not exist.

@RyanZim
Copy link
Collaborator

RyanZim commented May 10, 2021

How does fs.rm with force and/or recursive options set to true work? I'm wondering if this is a bug in Node itself.

@rhwood
Copy link
Author

rhwood commented May 13, 2021

I don't know. I may be doing something wrong.

On macOS 11.3.1 with Node.js 16.1.0,
this throws an error as expected:

import * as nodefs from 'fs'

nodefs.mkdirSync('tmp')
nodefs.writeFileSync('tmp/file', 'A file.')
nodefs.chmodSync('tmp', '500')
nodefs.rm('tmp/file', {force: true}, function(err) {
    console.log('Expected error message')
    if (err) {
        console.error(err)
    } else {
        console.log('Unexpected success')
    } 
})

but this does not (note same as above except after // clean up comment:

import * as nodefs from 'fs'

nodefs.mkdirSync('tmp')
nodefs.writeFileSync('tmp/file', 'A file.')
nodefs.chmodSync('tmp', '500')
nodefs.rm('tmp/file', {force: true}, function(err) {
    console.log('Expected error message')
    if (err) {
        console.error(err)
    } else {
        console.log('Unexpected success')
    } 
})
// clean up
nodefs.chmodSync('tmp', '700')
nodefs.rm('tmp', {recursive: true, force: true}, function(err) {
    if (err) {
        console.log('Unexpected error cleaning up')
        console.error(err)
    }
})

@RyanZim
Copy link
Collaborator

RyanZim commented May 13, 2021

but this does not

For clarity, what's the result of that code?

@rhwood
Copy link
Author

rhwood commented May 13, 2021

but this does not

For clarity, what's the result of that code?

Cleaning up...
Expected error message
Unexpected success

which means (I just realized) the rm is an asynchronous call.

@rhwood
Copy link
Author

rhwood commented May 13, 2021

I just submitted nodejs/node#38683 as Node does have this bug also.

@RyanZim
Copy link
Collaborator

RyanZim commented May 25, 2021

Closing as wontfix, since this is an upstream bug in Node. Node just merged a fix for this: nodejs/node#38684; from what I can tell, we should expect that fix to land in Node v17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants