From ea9e6bc1b8c6ba1df3bd85b25d36afe7540298ed Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Mon, 5 Aug 2019 16:48:10 -0400 Subject: [PATCH 1/5] Create/use global shared retry queue. Importing a fresh copy of graceful-fs then using `require('fs').close` would only initiate retry of the queue from the first copy of graceful-fs, so needed retries associated with subsequent instances of graceful-fs would not be executed. --- graceful-fs.js | 82 +++++++++++++++++++++++++------------------------- test/close.js | 22 +++++++++++--- 2 files changed, 58 insertions(+), 46 deletions(-) diff --git a/graceful-fs.js b/graceful-fs.js index ac20675..bc9c0e9 100644 --- a/graceful-fs.js +++ b/graceful-fs.js @@ -3,10 +3,10 @@ var polyfills = require('./polyfills.js') var legacy = require('./legacy-streams.js') var clone = require('./clone.js') -var queue = [] - var util = require('util') +var gracefulQueue = Symbol.for('graceful-fs.queue') + function noop () {} var debug = noop @@ -19,11 +19,44 @@ else if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) console.error(m) } -if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) { - process.on('exit', function() { - debug(queue) - require('assert').equal(queue.length, 0) +// Once time initialization +if (!global[gracefulQueue]) { + // This queue can be shared by multiple loaded instances + var queue = [] + Object.defineProperty(global, gracefulQueue, { + get: function() { + return queue + } }) + + // Patch fs.close/closeSync to shared queue version, because we need + // 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. + fs.close = (function (fs$close) { return function (fd, cb) { + return fs$close.call(fs, fd, function (err) { + // This function uses the graceful-fs shared queue + if (!err) { + retry() + } + + if (typeof cb === 'function') + cb.apply(this, arguments) + }) + }})(fs.close) + + fs.closeSync = (function (fs$closeSync) { return function (fd) { + // This function uses the graceful-fs shared queue + fs$closeSync.apply(fs, arguments) + retry() + }})(fs.closeSync) + + if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) { + process.on('exit', function() { + debug(global[gracefulQueue]) + require('assert').equal(global[gracefulQueue].length, 0) + }) + } } module.exports = patch(clone(fs)) @@ -32,39 +65,6 @@ if (process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH && !fs.__patched) { 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 = (function (fs$close) { return function (fd, cb) { - return fs$close.call(fs, fd, function (err) { - if (!err) - retry() - - if (typeof cb === 'function') - cb.apply(this, arguments) - }) -}})(fs.close) - -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 (!/\bgraceful-fs\b/.test(fs.closeSync.toString())) { - fs.closeSync = module.exports.closeSync; - fs.close = module.exports.close; -} - function patch (fs) { // Everything that references the open() function needs to be in here polyfills(fs) @@ -267,11 +267,11 @@ function patch (fs) { function enqueue (elem) { debug('ENQUEUE', elem[0].name, elem[1]) - queue.push(elem) + global[gracefulQueue].push(elem) } function retry () { - var elem = queue.shift() + var elem = global[gracefulQueue].shift() if (elem) { debug('RETRY', elem[0].name, elem[1]) elem[0].apply(null, elem[1]) diff --git a/test/close.js b/test/close.js index dd1f002..7210b31 100644 --- a/test/close.js +++ b/test/close.js @@ -1,10 +1,22 @@ -var fs$close = require('fs').close; -var fs$closeSync = require('fs').closeSync; -var fs = require('../'); +var fs = require('fs') +var path = require('path') +var gfsPath = path.resolve(__dirname, '..', 'graceful-fs.js') +var gfs = require(gfsPath) +var importFresh = require('import-fresh') +var fs$close = fs.close +var fs$closeSync = fs.closeSync var test = require('tap').test test('`close` is patched correctly', function(t) { - t.notEqual(fs.close, fs$close, 'patch close'); - t.notEqual(fs.closeSync, fs$closeSync, 'patch closeSync'); + t.match(fs$close.toString(), /graceful-fs shared queue/, 'patch fs.close'); + t.match(fs$closeSync.toString(), /graceful-fs shared queue/, 'patch fs.closeSync'); + t.match(gfs.close.toString(), /graceful-fs shared queue/, 'patch gfs.close'); + t.match(gfs.closeSync.toString(), /graceful-fs shared queue/, 'patch gfs.closeSync'); + + var newGFS = importFresh(gfsPath) + t.equal(fs.close, fs$close) + t.equal(fs.closeSync, fs$closeSync) + t.equal(newGFS.close, fs$close) + t.equal(newGFS.closeSync, fs$closeSync) t.end(); }) From 43ba0b14bbd1728a9a786b016d6cb8dd8cc8b92d Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Thu, 8 Aug 2019 15:44:17 -0400 Subject: [PATCH 2/5] Add test flag allowing reset of fs.close / fs.closeSync --- graceful-fs.js | 70 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/graceful-fs.js b/graceful-fs.js index bc9c0e9..b8acc06 100644 --- a/graceful-fs.js +++ b/graceful-fs.js @@ -6,6 +6,7 @@ var clone = require('./clone.js') var util = require('util') var gracefulQueue = Symbol.for('graceful-fs.queue') +var gracefulResetQueue = Symbol.for('graceful-fs.reset-queue') function noop () {} @@ -20,36 +21,59 @@ else if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) } // Once time initialization -if (!global[gracefulQueue]) { - // This queue can be shared by multiple loaded instances - var queue = [] - Object.defineProperty(global, gracefulQueue, { - get: function() { - return queue - } - }) +if (!global[gracefulQueue] || global[gracefulResetQueue]) { + global[gracefulResetQueue] = false + + if (!global[gracefulQueue]) { + // This queue can be shared by multiple loaded instances + var queue = [] + Object.defineProperty(global, gracefulQueue, { + get: function() { + return queue + } + }) + } + + var previous = Symbol.for('graceful-fs.previous') + if (fs.close[previous]) { + fs.close = fs.close[previous] + } + + if (fs.closeSync[previous]) { + fs.closeSync = fs.closeSync[previous] + } // Patch fs.close/closeSync to shared queue version, because we need // 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. - fs.close = (function (fs$close) { return function (fd, cb) { - return fs$close.call(fs, fd, function (err) { - // This function uses the graceful-fs shared queue - if (!err) { - retry() - } + fs.close = (function (fs$close) { + function close (fd, cb) { + return fs$close.call(fs, fd, function (err) { + // This function uses the graceful-fs shared queue + if (!err) { + retry() + } - if (typeof cb === 'function') - cb.apply(this, arguments) - }) - }})(fs.close) + if (typeof cb === 'function') + cb.apply(this, arguments) + }) + } + + close[previous] = fs$close + return close + })(fs.close) + + fs.closeSync = (function (fs$closeSync) { + function closeSync (fd) { + // This function uses the graceful-fs shared queue + fs$closeSync.apply(fs, arguments) + retry() + } - fs.closeSync = (function (fs$closeSync) { return function (fd) { - // This function uses the graceful-fs shared queue - fs$closeSync.apply(fs, arguments) - retry() - }})(fs.closeSync) + closeSync[previous] = fs$closeSync + return closeSync + })(fs.closeSync) if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) { process.on('exit', function() { From a91bd18ddf2be9c0dafcd09f3ec4bbc60dfc969f Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Thu, 8 Aug 2019 16:11:46 -0400 Subject: [PATCH 3/5] Remove changes not needed in v4.x Some changes in the previous commit are not needed in v4.x which will never be tested under nyc. The only thing needed is the ability for the next version of graceful-fs to reset fs.close / fs.closeSync. --- graceful-fs.js | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/graceful-fs.js b/graceful-fs.js index b8acc06..a644d57 100644 --- a/graceful-fs.js +++ b/graceful-fs.js @@ -6,7 +6,6 @@ var clone = require('./clone.js') var util = require('util') var gracefulQueue = Symbol.for('graceful-fs.queue') -var gracefulResetQueue = Symbol.for('graceful-fs.reset-queue') function noop () {} @@ -21,27 +20,17 @@ else if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) } // Once time initialization -if (!global[gracefulQueue] || global[gracefulResetQueue]) { - global[gracefulResetQueue] = false - - if (!global[gracefulQueue]) { - // This queue can be shared by multiple loaded instances - var queue = [] - Object.defineProperty(global, gracefulQueue, { - get: function() { - return queue - } - }) - } +if (!global[gracefulQueue]) { + // This queue can be shared by multiple loaded instances + var queue = [] + Object.defineProperty(global, gracefulQueue, { + get: function() { + return queue + } + }) + // This is used in testing by future versions var previous = Symbol.for('graceful-fs.previous') - if (fs.close[previous]) { - fs.close = fs.close[previous] - } - - if (fs.closeSync[previous]) { - fs.closeSync = fs.closeSync[previous] - } // Patch fs.close/closeSync to shared queue version, because we need // to retry() whenever a close happens *anywhere* in the program. From 6bb24d0642b03457aa9100368ef65f7b91545a2b Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Tue, 13 Aug 2019 20:49:01 -0400 Subject: [PATCH 4/5] Use Symbol.for fallback so really old versions of node.js aren't broken. --- graceful-fs.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/graceful-fs.js b/graceful-fs.js index a644d57..1e4128e 100644 --- a/graceful-fs.js +++ b/graceful-fs.js @@ -5,7 +5,15 @@ var clone = require('./clone.js') var util = require('util') -var gracefulQueue = Symbol.for('graceful-fs.queue') +var gracefulQueue +var previous +if (typeof Symbol === 'function' && typeof Symbol.for === 'function') { + gracefulQueue = Symbol.for('graceful-fs.queue') + previous = Symbol.for('graceful-fs.previous') +} else { + gracefulQueue = '@@symbol@@graceful-fs.queue' + previous = '@@symbol@@graceful-fs.previous' +} function noop () {} @@ -29,9 +37,6 @@ if (!global[gracefulQueue]) { } }) - // This is used in testing by future versions - var previous = Symbol.for('graceful-fs.previous') - // Patch fs.close/closeSync to shared queue version, because we need // to retry() whenever a close happens *anywhere* in the program. // This is essential when multiple graceful-fs instances are From b73606eed2b538b804f6ff0f1fc0c30c9b34daee Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Tue, 13 Aug 2019 20:55:16 -0400 Subject: [PATCH 5/5] Hide fs.close and fs.closeSync previous symbols --- graceful-fs.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/graceful-fs.js b/graceful-fs.js index 1e4128e..9488d05 100644 --- a/graceful-fs.js +++ b/graceful-fs.js @@ -54,7 +54,9 @@ if (!global[gracefulQueue]) { }) } - close[previous] = fs$close + Object.defineProperty(close, previous, { + value: fs$close + }) return close })(fs.close) @@ -65,7 +67,9 @@ if (!global[gracefulQueue]) { retry() } - closeSync[previous] = fs$closeSync + Object.defineProperty(closeSync, previous, { + value: fs$closeSync + }) return closeSync })(fs.closeSync)