Skip to content

Commit

Permalink
Do not inadvertently monkeypatch fs streams
Browse files Browse the repository at this point in the history
Fix: #170

Credit: @coreyfarrell
  • Loading branch information
isaacs committed Aug 14, 2019
1 parent bbb8e17 commit 9841d47
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 6 deletions.
51 changes: 45 additions & 6 deletions graceful-fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,7 @@ function patch (fs) {
// Everything that references the open() function needs to be in here
polyfills(fs)
fs.gracefulify = patch
fs.FileReadStream = ReadStream; // Legacy name.
fs.FileWriteStream = WriteStream; // Legacy name.

fs.createReadStream = createReadStream
fs.createWriteStream = createWriteStream
var fs$readFile = fs.readFile
Expand Down Expand Up @@ -213,8 +212,48 @@ function patch (fs) {
WriteStream.prototype.open = WriteStream$open
}

fs.ReadStream = ReadStream
fs.WriteStream = WriteStream
Object.defineProperty(fs, 'ReadStream', {

This comment has been minimized.

Copy link
@fcrick

fcrick Aug 27, 2019

Not sure why, but this change makes hasha.fromFile(anyPath) cause stack overflows - had to downgrade back to 4.2.1

This comment has been minimized.

Copy link
@coreyfarrell

coreyfarrell Aug 27, 2019

Collaborator

@fcrick I tried the following:

const gfs = require('graceful-fs');
if (process.env.GFS === 'patch') {
  gfs.gracefulify(require('fs'));
}
const hasha = require('hasha');

hasha.fromFile(__filename).then(console.log);

No stack errors in node.js 4, 6, 8, 10 or 12 with or without setting GFS=patch. In all cases the hash of the script was printed. Please open a new issue report including code demonstrating the issue with details (version of node.js, etc), tag my username in the new issue to be sure I see it.

This comment has been minimized.

Copy link
@jnardone

jnardone Aug 27, 2019

I can confirm something in this 4.2.2 build causes stack overflows - unfortunately I don't have a good direct test case. In our case graceful-fs is a downstream dependency (of @jest/core and other @jest repos most notably) and this triggers a lot of weirdness with Jest even when the Jest version is unchanged and only graceful-fs moves from 4.2.1 to 4.2.2

This comment has been minimized.

Copy link
@coreyfarrell

coreyfarrell Aug 27, 2019

Collaborator

@jnardone I wasn't doubting that @fcrick experienced an issue, I need more information to determine the cause and an appropriate fix. If I can't reproduce this issue then I'm not sure what to do.

This comment has been minimized.

Copy link
@isaacs

isaacs Aug 28, 2019

Author Owner

@jnardone @fcrick Do either of you have a repo I could clone and poke at this bug myself?

get: function () {
return ReadStream
},
set: function (val) {
ReadStream = val
},
enumerable: true,
configurable: true
})
Object.defineProperty(fs, 'WriteStream', {
get: function () {
return WriteStream
},
set: function (val) {
WriteStream = val
},
enumerable: true,
configurable: true
})

// legacy names
Object.defineProperty(fs, 'FileReadStream', {
get: function () {
return ReadStream
},
set: function (val) {
ReadStream = val
},
enumerable: true,
configurable: true
})
Object.defineProperty(fs, 'FileWriteStream', {
get: function () {
return WriteStream
},
set: function (val) {
WriteStream = val
},
enumerable: true,
configurable: true
})

function ReadStream (path, options) {
if (this instanceof ReadStream)
Expand Down Expand Up @@ -260,11 +299,11 @@ function patch (fs) {
}

function createReadStream (path, options) {
return new ReadStream(path, options)
return new fs.ReadStream(path, options)
}

function createWriteStream (path, options) {
return new WriteStream(path, options)
return new fs.WriteStream(path, options)
}

var fs$open = fs.open
Expand Down
31 changes: 31 additions & 0 deletions test/monkeypatch-by-accident.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

if (process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH) {
require('tap').plan(0, 'obviously not relevant when monkeypatching fs')
process.exit(0)
}

const fs = require('fs')

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

const t = require('tap')
const gfs = require('../')

if (names.some(name => gfs[name] === orig[name])) {
t.plan(0, 'graceful-fs was loaded before this test was run')
process.exit(0)
}

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

0 comments on commit 9841d47

Please sign in to comment.