fix memory leaks, includes test to make sure fix is working

.travis.yml

@@ -5,3 +5,8 @@ node_js:
   - '4'
   - '5'
   - '6'
+  - '7'
+  - '8'
+  - '9'
+  - '10'
+  - '11'

fs.js → clone.js

@@ -1,8 +1,6 @@
 'use strict'
 
-var fs = require('fs')
-
-module.exports = clone(fs)
+module.exports = clone
 
 function clone (obj) {
   if (obj === null || typeof obj !== 'object')

graceful-fs.js

@@ -1,6 +1,8 @@
 var fs = require('fs')
+var clone = require('./clone.js')
 var polyfills = require('./polyfills.js')
 var legacy = require('./legacy-streams.js')
+
 var queue = []
 
 var util = require('util')
@@ -24,17 +26,17 @@ if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) {
   })
 }
 
-module.exports = patch(require('./fs.js'))
-if (process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH) {
-  module.exports = patch(fs)
+module.exports = patch(clone(fs))
+if (process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH && !fs.__patched) {
+    module.exports = patch(fs)
+    fs.__patched = true;
 }
 
 // Always patch fs.close/closeSync, because we want to
 // retry() whenever a close happens *anywhere* in the program.
 // This is essential when multiple graceful-fs instances are
 // in play at the same time.
-module.exports.close =
-fs.close = (function (fs$close) { return function (fd, cb) {
+module.exports.close = (function (fs$close) { return function (fd, cb) {
   return fs$close.call(fs, fd, function (err) {
     if (!err)
       retry()
@@ -44,17 +46,27 @@ fs.close = (function (fs$close) { return function (fd, cb) {
   })
 }})(fs.close)
 
-module.exports.closeSync =
-fs.closeSync = (function (fs$closeSync) { return function (fd) {
+module.exports.closeSync = (function (fs$closeSync) { return function (fd) {
   // Note that graceful-fs also retries when fs.closeSync() fails.
   // Looks like a bug to me, although it's probably a harmless one.
   var rval = fs$closeSync.apply(fs, arguments)
   retry()
   return rval
 }})(fs.closeSync)
 
+// Only patch fs once, otherwise we'll run into a memory leak if
+// graceful-fs is loaded multiple times, such as in test environments that
+// reset the loaded modules between tests.
+// We look for the string `graceful-fs` from the comment above. This
+// way we are not adding any extra properties and it will detect if older
+// versions of graceful-fs are installed.
+if (!fs.closeSync.toString().includes('graceful-fs')) {
+    fs.closeSync = module.exports.closeSync;
+    fs.close = module.exports.close;
+}
+
 function patch (fs) {
-  // Everything that references the open() function needs to be in here
+    // Everything that references the open() function needs to be in here
   polyfills(fs)
   fs.gracefulify = patch
   fs.FileReadStream = ReadStream;  // Legacy name.
@@ -142,8 +154,9 @@ function patch (fs) {
       if (files && files.sort)
         files.sort()
 
-      if (err && (err.code === 'EMFILE' || err.code === 'ENFILE'))
-        enqueue([go$readdir, [args]])
+      if (err && (err.code === 'EMFILE' || err.code === 'ENFILE')) {
+          enqueue([go$readdir, [args]])
+      }
       else {
         if (typeof cb === 'function')
           cb.apply(this, arguments)

package.json

@@ -34,6 +34,7 @@
   ],
   "license": "ISC",
   "devDependencies": {
+    "import-fresh": "^2.0.0",
     "mkdirp": "^0.5.0",
     "rimraf": "^2.2.8",
     "tap": "^11.0.0"

test.js

@@ -1,4 +1,3 @@
-var spawn = require('child_process').spawn
 var fs = require('fs')
 var tap = require('tap')
 var dir = __dirname + '/test'
@@ -14,11 +13,12 @@ var env = Object.keys(process.env).reduce(function (env, k) {
 
 files.filter(function (f) {
   if (/\.js$/.test(f) && fs.statSync(dir + '/' + f).isFile()) {
-    tap.spawn(node, ['test/' + f])
+    // expose-gc is so we can check for memory leaks
+    tap.spawn(node, ['--expose-gc', 'test/' + f])
     return true
   }
 }).forEach(function (f) {
-  tap.spawn(node, ['test/' + f], {
+  tap.spawn(node, ['--expose-gc', 'test/' + f], {
     env: env
   }, '🐵  test/' + f)
 })

test/avoid-memory-leak.js

@@ -0,0 +1,25 @@
+var importFresh = require('import-fresh');
+var t = require('tap')
+
+t.test('no memory leak when loading multiple times', function(t) {
+    t.plan(1);
+    importFresh('../')
+    const mbUsedBefore = process.memoryUsage().heapUsed / 1024 ** 2;
+    // simulate project with 4000 tests
+    var i = 0;
+    function importFreshGracefulFs() {
+        importFresh('../');
+        if (i < 4000) {
+            i++;
+            process.nextTick(() => importFreshGracefulFs());
+        } else {
+            global.gc();
+            const mbUsedAfter = process.memoryUsage().heapUsed / 1024 ** 2;
+            // We expect less than a 2 MB difference
+            const memoryUsageMB = Math.round(mbUsedAfter - mbUsedBefore);
+            t.ok(memoryUsageMB < 2, 'We expect less than 2MB difference, but ' + memoryUsageMB + 'MB is still claimed.');
+            t.end();
+        }
+    }
+    importFreshGracefulFs();
+})

test/readdir-options.js

@@ -29,7 +29,7 @@ fs.readdir = function(path, options, cb) {
       er.code = 'EMFILE'
       cb(er)
       process.nextTick(function () {
-        fs.closeSync(fs.openSync(__filename, 'r'))
+        g.closeSync(fs.openSync(__filename, 'r'))
       })
     })
     return