From cbb3a0d2cd0af7c010d086c6fd9f0aa9c0d8fb31 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 20 Sep 2026 21:14:47 +0000 Subject: [PATCH 1/2] fix: do not throw when fs is not extensible Store the shared retry queue off the fs object when it is not extensible (ESM node:fs namespace), while keeping the existing Symbol-on-fs publish path when Object.isExtensible(fs) is true. Fixes #245 Co-authored-by: David --- graceful-fs.js | 54 +++++++++++++------ test/non-extensible-fs.js | 98 +++++++++++++++++++++++++++++++++++ test/queue-published-on-fs.js | 21 ++++++++ 3 files changed, 156 insertions(+), 17 deletions(-) create mode 100644 test/non-extensible-fs.js create mode 100644 test/queue-published-on-fs.js diff --git a/graceful-fs.js b/graceful-fs.js index 8d5b89e..8cd088c 100644 --- a/graceful-fs.js +++ b/graceful-fs.js @@ -22,11 +22,29 @@ if (typeof Symbol === 'function' && typeof Symbol.for === 'function') { function noop () {} function publishQueue(context, queue) { - Object.defineProperty(context, gracefulQueue, { - get: function() { - return queue - } - }) + // ESM `node:fs` namespace objects are not extensible (#245). + // Keep the Symbol-on-fs / Symbol-on-global sharing path when possible. + if (!context || (typeof Object.isExtensible === 'function' && !Object.isExtensible(context))) { + return + } + try { + Object.defineProperty(context, gracefulQueue, { + get: function() { + return queue + } + }) + } catch (err) { + // ignore sealed / host objects that still reject the new property + } +} + +// Shared retry queue. Prefer the published Symbol on `fs` so multiple +// copies of this module see the same array. When `fs` is not extensible, +// fall back to global (already used for cross-version sharing) and then +// a module-local array so loading never throws. +var fallbackQueue = [] +function getQueue () { + return fs[gracefulQueue] || global[gracefulQueue] || fallbackQueue } var debug = noop @@ -42,7 +60,7 @@ else if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) // Once time initialization if (!fs[gracefulQueue]) { // This queue can be shared by multiple loaded instances - var queue = global[gracefulQueue] || [] + var queue = global[gracefulQueue] || fallbackQueue publishQueue(fs, queue) // Patch fs.close/closeSync to shared queue version, because we need @@ -83,14 +101,14 @@ if (!fs[gracefulQueue]) { if (/\bgfs4\b/i.test(process.env.NODE_DEBUG || '')) { process.on('exit', function() { - debug(fs[gracefulQueue]) - require('assert').equal(fs[gracefulQueue].length, 0) + debug(getQueue()) + require('assert').equal(getQueue().length, 0) }) } } if (!global[gracefulQueue]) { - publishQueue(global, fs[gracefulQueue]); + publishQueue(global, getQueue()); } module.exports = patch(clone(fs)) @@ -370,7 +388,7 @@ function patch (fs) { function enqueue (elem) { debug('ENQUEUE', elem[0].name, elem[1]) - fs[gracefulQueue].push(elem) + getQueue().push(elem) retry() } @@ -382,12 +400,13 @@ var retryTimer // delay between attempts so that we'll retry these jobs sooner function resetQueue () { var now = Date.now() - for (var i = 0; i < fs[gracefulQueue].length; ++i) { + var queue = getQueue() + for (var i = 0; i < queue.length; ++i) { // entries that are only a length of 2 are from an older version, don't // bother modifying those since they'll be retried anyway. - if (fs[gracefulQueue][i].length > 2) { - fs[gracefulQueue][i][3] = now // startTime - fs[gracefulQueue][i][4] = now // lastTime + if (queue[i].length > 2) { + queue[i][3] = now // startTime + queue[i][4] = now // lastTime } } // call retry to make sure we're actively processing the queue @@ -399,10 +418,11 @@ function retry () { clearTimeout(retryTimer) retryTimer = undefined - if (fs[gracefulQueue].length === 0) + var queue = getQueue() + if (queue.length === 0) return - var elem = fs[gracefulQueue].shift() + var elem = queue.shift() var fn = elem[0] var args = elem[1] // these items may be unset if they were added by an older graceful-fs @@ -437,7 +457,7 @@ function retry () { } else { // if we can't do this job yet, push it to the end of the queue // and let the next iteration check again - fs[gracefulQueue].push(elem) + queue.push(elem) } } diff --git a/test/non-extensible-fs.js b/test/non-extensible-fs.js new file mode 100644 index 0000000..de7498a --- /dev/null +++ b/test/non-extensible-fs.js @@ -0,0 +1,98 @@ +'use strict' + +// Regression for https://github.com/isaacs/node-graceful-fs/issues/245 +// +// In ESM, `import * as fs from 'node:fs'` is a non-extensible namespace +// object. graceful-fs used to throw: +// TypeError: Cannot define property Symbol(graceful-fs.queue), object is not extensible +// +// This test simulates that by Object.preventExtensions on the CJS fs +// export (methods stay writable, which isolates #245 from getter-only +// close assignment). Global-patch mode is skipped because that path +// assigns `fs.__patched`, a new property. + +if (process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH) { + require('tap').plan(0, 'non-extensible fs coverage is for the default export path') + process.exit(0) +} + +var fs = require('fs') +var path = require('path') +var importFresh = require('import-fresh') +var test = require('tap').test + +var gfsPath = path.resolve(__dirname, '..', 'graceful-fs.js') +var queueSymbol = typeof Symbol === 'function' && typeof Symbol.for === 'function' + ? Symbol.for('graceful-fs.queue') + : '___graceful-fs.queue' + +test('precondition: CJS fs starts extensible', function (t) { + t.equal(Object.isExtensible(fs), true) + t.end() +}) + +test('loading must not throw when fs is not extensible', function (t) { + Object.preventExtensions(fs) + t.equal(Object.isExtensible(fs), false) + + var gfs + t.doesNotThrow(function () { + gfs = require(gfsPath) + }, 'require(graceful-fs) does not throw') + + t.type(gfs.readFile, 'function') + t.type(gfs.openSync, 'function') + t.type(gfs.closeSync, 'function') + + t.equal(fs[queueSymbol], undefined, + 'does not define the queue Symbol on a non-extensible fs') + t.ok(Array.isArray(global[queueSymbol]), + 'queue is stored off fs (on global) so copies can still share it') + + var fd = gfs.openSync(__filename, 'r') + gfs.closeSync(fd) + + gfs.readFile(__filename, 'utf8', function (err, data) { + t.error(err, 'readFile still works') + t.match(data, /not extensible/) + t.end() + }) +}) + +test('import-fresh copies share the same queue via global', function (t) { + var q1 = global[queueSymbol] + t.ok(Array.isArray(q1)) + + var gfs2 = importFresh(gfsPath) + t.type(gfs2.readFile, 'function') + t.equal(global[queueSymbol], q1, + 'second load keeps the shared queue instance') + t.equal(fs[queueSymbol], undefined) + t.end() +}) + +test('EMFILE retries use the off-fs queue', function (t) { + var readFile = fs.readFile + var realNow = Date.now + var EMFILE = Object.assign(new Error('FAKE EMFILE'), { code: 'EMFILE' }) + + t.teardown(function () { + fs.readFile = readFile + Date.now = realNow + }) + + fs.readFile = function (p, options, cb) { + process.nextTick(function () { + cb(EMFILE) + Date.now = function () { + return realNow() + 60000 + } + }) + } + + var gfs = importFresh(gfsPath) + gfs.readFile('literally anything', function (err) { + t.equal(err.code, 'EMFILE', 'eventually got the EMFILE from the shared queue') + t.end() + }) +}) diff --git a/test/queue-published-on-fs.js b/test/queue-published-on-fs.js new file mode 100644 index 0000000..f55c033 --- /dev/null +++ b/test/queue-published-on-fs.js @@ -0,0 +1,21 @@ +'use strict' + +// Lock the historical CJS sharing path: when `fs` is extensible, the +// retry queue is published on `fs` via Symbol.for('graceful-fs.queue') +// so multiple graceful-fs copies see the same array (#245 fallback +// must not change this). + +var fs = require('fs') +var t = require('tap') + +var queueSymbol = typeof Symbol === 'function' && typeof Symbol.for === 'function' + ? Symbol.for('graceful-fs.queue') + : '___graceful-fs.queue' + +t.ok(Object.isExtensible(fs), 'CJS fs is extensible') + +var gfs = require('../') +t.type(gfs.readFile, 'function') +t.ok(Array.isArray(fs[queueSymbol]), 'queue published on fs when extensible') +t.ok(Array.isArray(global[queueSymbol]), 'queue also published on global') +t.equal(fs[queueSymbol], global[queueSymbol], 'fs and global share one queue array') From 9a9f1649c815a2c3cc2e48a51ea9a6e2af1c45cd Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 20 Sep 2026 21:18:15 +0000 Subject: [PATCH 2/2] test: isolate non-extensible fs regression from nyc preload nyc loads node_modules/graceful-fs before tests, which already publishes the queue Symbol on the real fs. Load this tree against a cloned, Object.preventExtensions fs-like object instead. Co-authored-by: David --- test/non-extensible-fs.js | 81 ++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 30 deletions(-) diff --git a/test/non-extensible-fs.js b/test/non-extensible-fs.js index de7498a..cb05d00 100644 --- a/test/non-extensible-fs.js +++ b/test/non-extensible-fs.js @@ -6,82 +6,102 @@ // object. graceful-fs used to throw: // TypeError: Cannot define property Symbol(graceful-fs.queue), object is not extensible // -// This test simulates that by Object.preventExtensions on the CJS fs -// export (methods stay writable, which isolates #245 from getter-only -// close assignment). Global-patch mode is skipped because that path -// assigns `fs.__patched`, a new property. - -if (process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH) { - require('tap').plan(0, 'non-extensible fs coverage is for the default export path') - process.exit(0) -} +// nyc (via package-hash) preloads node_modules/graceful-fs onto the real +// `fs` before tests run, so we load *this* tree's graceful-fs.js against a +// cloned, Object.preventExtensions fs-like object instead. -var fs = require('fs') +var Module = require('module') var path = require('path') -var importFresh = require('import-fresh') +var clone = require('../clone.js') +var realFs = require('fs') var test = require('tap').test -var gfsPath = path.resolve(__dirname, '..', 'graceful-fs.js') +var filename = path.resolve(__dirname, '..', 'graceful-fs.js') var queueSymbol = typeof Symbol === 'function' && typeof Symbol.for === 'function' ? Symbol.for('graceful-fs.queue') : '___graceful-fs.queue' -test('precondition: CJS fs starts extensible', function (t) { - t.equal(Object.isExtensible(fs), true) - t.end() -}) +function loadGracefulFs (fakeFs) { + var prev = process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH + delete process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH + + var m = new Module(filename + '-non-extensible-' + Date.now() + '-' + Math.random(), module) + m.filename = filename + m.paths = Module._nodeModulePaths(path.dirname(filename)) + var protoRequire = Module.prototype.require + m.require = function (request) { + if (request === 'fs') + return fakeFs + return protoRequire.call(this, request) + } + + try { + m.load(filename) + return m.exports + } finally { + if (prev !== undefined) + process.env.TEST_GRACEFUL_FS_GLOBAL_PATCH = prev + } +} + +function nonExtensibleFs () { + var fakeFs = clone(realFs) + Object.preventExtensions(fakeFs) + return fakeFs +} test('loading must not throw when fs is not extensible', function (t) { - Object.preventExtensions(fs) - t.equal(Object.isExtensible(fs), false) + var fakeFs = nonExtensibleFs() + t.equal(Object.isExtensible(fakeFs), false) var gfs t.doesNotThrow(function () { - gfs = require(gfsPath) - }, 'require(graceful-fs) does not throw') + gfs = loadGracefulFs(fakeFs) + }, 'loading graceful-fs does not throw') t.type(gfs.readFile, 'function') t.type(gfs.openSync, 'function') t.type(gfs.closeSync, 'function') - t.equal(fs[queueSymbol], undefined, + t.equal(fakeFs[queueSymbol], undefined, 'does not define the queue Symbol on a non-extensible fs') t.ok(Array.isArray(global[queueSymbol]), 'queue is stored off fs (on global) so copies can still share it') - var fd = gfs.openSync(__filename, 'r') + var fd = gfs.openSync(filename, 'r') gfs.closeSync(fd) - gfs.readFile(__filename, 'utf8', function (err, data) { + gfs.readFile(filename, 'utf8', function (err, data) { t.error(err, 'readFile still works') t.match(data, /not extensible/) t.end() }) }) -test('import-fresh copies share the same queue via global', function (t) { +test('later loads share the same queue via global', function (t) { + var fakeFs = nonExtensibleFs() + loadGracefulFs(fakeFs) var q1 = global[queueSymbol] t.ok(Array.isArray(q1)) - var gfs2 = importFresh(gfsPath) + var gfs2 = loadGracefulFs(fakeFs) t.type(gfs2.readFile, 'function') t.equal(global[queueSymbol], q1, 'second load keeps the shared queue instance') - t.equal(fs[queueSymbol], undefined) + t.equal(fakeFs[queueSymbol], undefined) t.end() }) test('EMFILE retries use the off-fs queue', function (t) { - var readFile = fs.readFile var realNow = Date.now var EMFILE = Object.assign(new Error('FAKE EMFILE'), { code: 'EMFILE' }) + var fakeFs = clone(realFs) t.teardown(function () { - fs.readFile = readFile Date.now = realNow }) - fs.readFile = function (p, options, cb) { + fakeFs.readFile = function (p, options, cb) { process.nextTick(function () { cb(EMFILE) Date.now = function () { @@ -89,8 +109,9 @@ test('EMFILE retries use the off-fs queue', function (t) { } }) } + Object.preventExtensions(fakeFs) - var gfs = importFresh(gfsPath) + var gfs = loadGracefulFs(fakeFs) gfs.readFile('literally anything', function (err) { t.equal(err.code, 'EMFILE', 'eventually got the EMFILE from the shared queue') t.end()