search

jprichardson / node-fs-extra

Node.js: extra methods for the fs object like copy(), remove(), mkdirs()
MIT License
9.47k stars 772 forks source link

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

Closed rhwood closed 3 years ago

rhwood commented 3 years ago

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

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

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

but this does not

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

rhwood commented 3 years ago

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

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

RyanZim commented 3 years ago

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