isaacs / node-graceful-fs

fs with incremental backoff on EMFILE
ISC License
1.27k stars 148 forks source link

fs.*Stream classes are (inadvertantly?) monkey-patched in current versions #170

Closed coreyfarrell closed 5 years ago

coreyfarrell commented 5 years ago

fs is cloned by copying property descriptors to a new object. We replace the stream classes:

fs.FileReadStream = ReadStream;  // Legacy name.
fs.FileWriteStream = WriteStream;  // Legacy name.

And later:

fs.ReadStream = ReadStream
fs.WriteStream = WriteStream

Unfortunately these classes are defined in the node.js fs module using property getter/setter functions. The getter/setter from the fs module are copied to the cloned object, so we are actually updating the internal fs variable.

The fix for this is to set the updated values using Object.defineProperty with an updated get/set mimicking the core fs module but pointing to a gfs variable. Not sure if this would be considered a breaking change for gfs? On specific way this can be a real issue is fs.WriteStream.open does not honor the autoClose: false option which was added in node.js 5.

Failing test-case:

'use strict';

const fs = require('fs');
const t = require('tap');

// Save originals before loading graceful-fs
const names = ['ReadStream', 'WriteStream', 'FileReadStream', 'FileWriteStream'];
const orig = names.reduce((orig, name) => Object.assign(orig, {[name]: fs[name]}), {});

const gfs = require('graceful-fs');

if (names.some(name => gfs[name] === orig[name])) {
  t.fail('This test cannot be run under nyc');
  process.exit(1);
}

names.forEach(name => {
  t.ok(fs[name] === orig[name], `fs.${name} unchanged`);
});

fs.createReadStream and fs.createWriteStream are not monkey-patched like this but they are effected as they use fs.ReadStream and fs.WriteStream.