From 5889bb8a6ee2aa980b9ce1f34a51bf008a906d83 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 15 Sep 2021 19:30:14 +0200 Subject: [PATCH 1/5] Add new ReadFileContext patch that adds support for Node.js v16.3.0+ (#332) --- lib/binding.js | 2 +- lib/error.js | 13 +++- lib/filesystem.js | 2 +- lib/index.js | 21 ++++++- lib/readfilecontext.js | 135 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 169 insertions(+), 4 deletions(-) create mode 100644 lib/readfilecontext.js diff --git a/lib/binding.js b/lib/binding.js index 1a1d7ee8..9fb12f73 100644 --- a/lib/binding.js +++ b/lib/binding.js @@ -6,7 +6,7 @@ const File = require('./file'); const FileDescriptor = require('./descriptor'); const Directory = require('./directory'); const SymbolicLink = require('./symlink'); -const FSError = require('./error'); +const {FSError} = require('./error'); const constants = require('constants'); const getPathParts = require('./filesystem').getPathParts; diff --git a/lib/error.js b/lib/error.js index 15366407..a5751fda 100644 --- a/lib/error.js +++ b/lib/error.js @@ -40,7 +40,18 @@ function FSError(code, path) { FSError.prototype = new Error(); FSError.codes = codes; +/** + * Creates an abort error for when an asynchronous task was aborted. + */ +function AbortError() { + Error.call(this); + this.code = 'ABORT_ERR'; + this.name = 'AbortError'; + Error.captureStackTrace(this, AbortError); +} + /** * Error constructor. */ -exports = module.exports = FSError; +exports.FSError = FSError; +exports.AbortError = AbortError; diff --git a/lib/filesystem.js b/lib/filesystem.js index 268a8b2e..de59019d 100644 --- a/lib/filesystem.js +++ b/lib/filesystem.js @@ -5,7 +5,7 @@ const path = require('path'); const Directory = require('./directory'); const File = require('./file'); -const FSError = require('./error'); +const {FSError} = require('./error'); const SymbolicLink = require('./symlink'); const isWindows = process.platform === 'win32'; diff --git a/lib/index.js b/lib/index.js index 3a9f1eab..a4eea19b 100644 --- a/lib/index.js +++ b/lib/index.js @@ -1,12 +1,16 @@ 'use strict'; const Binding = require('./binding'); -const FSError = require('./error'); +const {FSError} = require('./error'); const FileSystem = require('./filesystem'); const realBinding = process.binding('fs'); const path = require('path'); const loader = require('./loader'); const bypass = require('./bypass'); +const { + getReadFileContextPrototype, + patchReadFileContext +} = require('./readfilecontext'); const fs = require('fs'); const toNamespacedPath = FileSystem.toNamespacedPath; @@ -50,6 +54,10 @@ for (const key in Binding.prototype) { } } +const readFileContextPrototype = getReadFileContextPrototype(); + +patchReadFileContext(readFileContextPrototype); + function overrideBinding(binding) { realBinding._mockedBinding = binding; } @@ -94,6 +102,10 @@ function overrideCreateWriteStream() { }; } +function overrideReadFileContext(binding) { + readFileContextPrototype._mockedBinding = binding; +} + function restoreBinding() { delete realBinding._mockedBinding; realBinding.Stats = realStats; @@ -110,6 +122,10 @@ function restoreCreateWriteStream() { fs.createWriteStream = realCreateWriteStream; } +function restoreReadFileContext(binding) { + delete readFileContextPrototype._mockedBinding; +} + /** * Swap out the fs bindings for a mock file system. * @param {Object} config Mock file system configuration. @@ -125,6 +141,8 @@ exports = module.exports = function mock(config, options) { overrideBinding(binding); + overrideReadFileContext(binding); + let currentPath = process.cwd(); overrideProcess( function cwd() { @@ -167,6 +185,7 @@ exports.restore = function() { restoreBinding(); restoreProcess(); restoreCreateWriteStream(); + restoreReadFileContext(); }; /** diff --git a/lib/readfilecontext.js b/lib/readfilecontext.js new file mode 100644 index 00000000..69850826 --- /dev/null +++ b/lib/readfilecontext.js @@ -0,0 +1,135 @@ +'use strict'; + +const {AbortError} = require('./error'); +const {FSReqCallback} = process.binding('fs'); + +const kReadFileUnknownBufferLength = 64 * 1024; +const kReadFileBufferLength = 512 * 1024; + +function getReadFileContextPrototype() { + const fs = require('fs'); + const fsBinding = process.binding('fs'); + + const originalOpen = fsBinding.open; + + let proto; + fsBinding.open = (_path, _flags, _mode, req) => { + proto = Object.getPrototypeOf(req.context); + }; + + fs.readFile('/ignored.txt', () => {}); + + fsBinding.open = originalOpen; + + return proto; +} + +function patchReadFileContext(prototype) { + const origRead = prototype.read; + const origClose = prototype.close; + + function readFileAfterRead(err, bytesRead) { + const context = this.context; + + if (err) { + return context.close(err); + } + context.pos += bytesRead; + + if (context.pos === context.size || bytesRead === 0) { + context.close(); + } else { + if (context.size === 0) { + // Unknown size, just read until we don't get bytes. + const buffer = + bytesRead === kReadFileUnknownBufferLength + ? context.buffer + : context.buffer.slice(0, bytesRead); + Array.prototype.push.apply(context.buffers, buffer); + } + context.read(); + } + } + + function readFileAfterClose(err) { + const context = this.context; + const callback = context.callback; + let buffer = null; + + if (context.err || err) { + return callback(context.err || err); + } + + try { + if (context.size === 0) { + buffer = Buffer.concat(context.buffers, context.pos); + } else if (context.pos < context.size) { + buffer = context.buffer.slice(0, context.pos); + } else { + buffer = context.buffer; + } + + if (context.encoding) { + buffer = buffer.toString(context.encoding); + } + } catch (err) { + return callback(err); + } + + callback(null, buffer); + } + + prototype.read = function read() { + if (!prototype._mockedBinding) { + return origRead.apply(this, arguments); + } + + let buffer; + let offset; + let length; + + if (this.signal && this.signal.aborted) { + return this.close(new AbortError()); + } + if (this.size === 0) { + buffer = Buffer.allocUnsafeSlow(kReadFileUnknownBufferLength); + offset = 0; + length = kReadFileUnknownBufferLength; + this.buffer = buffer; + } else { + buffer = this.buffer; + offset = this.pos; + length = Math.min(kReadFileBufferLength, this.size - this.pos); + } + + const req = new FSReqCallback(); + req.oncomplete = readFileAfterRead; + req.context = this; + + prototype._mockedBinding.read(this.fd, buffer, offset, length, -1, req); + }; + + prototype.close = function close(err) { + if (!prototype._mockedBinding) { + return origClose.apply(this, arguments); + } + + if (this.isUserFd) { + process.nextTick(function tick(context) { + readFileAfterClose.apply({context}, [null]); + }, this); + return; + } + + const req = new FSReqCallback(); + req.oncomplete = readFileAfterClose; + req.context = this; + this.err = err; + + prototype._mockedBinding.close(this.fd, req); + }; +} + +exports.patchReadFileContext = patchReadFileContext; + +exports.getReadFileContextPrototype = getReadFileContextPrototype; From 14e6eaa6db8b43b93223e1e71de095fb45e56dd3 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 15 Sep 2021 19:43:10 +0200 Subject: [PATCH 2/5] readfilecontext: bit more documentation and tweak code style --- lib/error.js | 8 ++++++-- lib/readfilecontext.js | 41 +++++++++++++++++++++++++++++------------ 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/lib/error.js b/lib/error.js index a5751fda..d4c3a1be 100644 --- a/lib/error.js +++ b/lib/error.js @@ -41,7 +41,8 @@ FSError.prototype = new Error(); FSError.codes = codes; /** - * Creates an abort error for when an asynchronous task was aborted. + * Create an abort error for when an asynchronous task was aborted. + * @constructor */ function AbortError() { Error.call(this); @@ -51,7 +52,10 @@ function AbortError() { } /** - * Error constructor. + * FSError constructor. */ exports.FSError = FSError; +/** + * AbortError constructor. + */ exports.AbortError = AbortError; diff --git a/lib/readfilecontext.js b/lib/readfilecontext.js index 69850826..699b6cbb 100644 --- a/lib/readfilecontext.js +++ b/lib/readfilecontext.js @@ -3,10 +3,12 @@ const {AbortError} = require('./error'); const {FSReqCallback} = process.binding('fs'); -const kReadFileUnknownBufferLength = 64 * 1024; -const kReadFileBufferLength = 512 * 1024; - -function getReadFileContextPrototype() { +/** + * This is a workaround for getting access to the ReadFileContext + * prototype, which we need to be able to patch its methods. + * @returns {object} + */ +exports.getReadFileContextPrototype = function() { const fs = require('fs'); const fsBinding = process.binding('fs'); @@ -22,12 +24,27 @@ function getReadFileContextPrototype() { fsBinding.open = originalOpen; return proto; -} - -function patchReadFileContext(prototype) { +}; + +/** + * This patches the ReadFileContext prototype to use mocked bindings + * when available. This entire implementation is more or less fully + * copied over from Node.js's /lib/internal/fs/read_file_context.js + * + * This patch is required to support Node.js v16+, where the ReadFileContext + * closes directly over the internal fs bindings, and is also eagerly loader. + * + * See https://github.com/tschaub/mock-fs/issues/332 for more information. + * + * @param {object} prototype The ReadFileContext prototype object to patch. + */ +exports.patchReadFileContext = function(prototype) { const origRead = prototype.read; const origClose = prototype.close; + const kReadFileUnknownBufferLength = 64 * 1024; + const kReadFileBufferLength = 512 * 1024; + function readFileAfterRead(err, bytesRead) { const context = this.context; @@ -57,6 +74,7 @@ function patchReadFileContext(prototype) { let buffer = null; if (context.err || err) { + // This is a simplification from Node.js, where we don't bother merging the errors return callback(context.err || err); } @@ -106,6 +124,9 @@ function patchReadFileContext(prototype) { req.oncomplete = readFileAfterRead; req.context = this; + // This call and the one in close() is what we want to change, the + // rest is pretty much the same as Node.js except we don't have access + // to some of the internal optimizations. prototype._mockedBinding.read(this.fd, buffer, offset, length, -1, req); }; @@ -128,8 +149,4 @@ function patchReadFileContext(prototype) { prototype._mockedBinding.close(this.fd, req); }; -} - -exports.patchReadFileContext = patchReadFileContext; - -exports.getReadFileContextPrototype = getReadFileContextPrototype; +}; From 73b015edc071240890b3df80fa5829b1838444d4 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 17 Sep 2021 11:31:57 +0200 Subject: [PATCH 3/5] readfilecontext: added tests to make sure patching happens correctly --- test/lib/readfilecontext.js | 107 ++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 test/lib/readfilecontext.js diff --git a/test/lib/readfilecontext.js b/test/lib/readfilecontext.js new file mode 100644 index 00000000..a5bb5be1 --- /dev/null +++ b/test/lib/readfilecontext.js @@ -0,0 +1,107 @@ +'use strict'; + +const helper = require('../helper'); +const { + patchReadFileContext, + getReadFileContextPrototype +} = require('../../lib/readfilecontext'); + +const assert = helper.assert; + +describe('getReadFileContextPrototype', function() { + it('provides access to the internal ReadFileContext', function() { + const proto = getReadFileContextPrototype(); + assert.equal(proto.constructor.name, 'ReadFileContext'); + assert.equal(typeof proto.read, 'function'); + assert.equal(typeof proto.close, 'function'); + }); +}); + +describe('patchReadFileContext', function() { + it('patch forwards calls to mocked binding when available', function() { + const calls = { + read: 0, + close: 0, + mockedRead: 0, + mockedClose: 0 + }; + + const proto = { + read: function() { + calls.read++; + }, + close: function() { + calls.close++; + } + }; + + const mockedBinding = { + read: function() { + assert.strictEqual(this, mockedBinding); + calls.mockedRead++; + }, + close: function() { + assert.strictEqual(this, mockedBinding); + calls.mockedClose++; + } + }; + + patchReadFileContext(proto); + + const target = Object.create(proto); + + assert.deepEqual(calls, { + read: 0, + close: 0, + mockedRead: 0, + mockedClose: 0 + }); + + target.read(); + assert.deepEqual(calls, { + read: 1, + close: 0, + mockedRead: 0, + mockedClose: 0 + }); + target.close(); + assert.deepEqual(calls, { + read: 1, + close: 1, + mockedRead: 0, + mockedClose: 0 + }); + + proto._mockedBinding = mockedBinding; + target.read(); + assert.deepEqual(calls, { + read: 1, + close: 1, + mockedRead: 1, + mockedClose: 0 + }); + target.close(); + assert.deepEqual(calls, { + read: 1, + close: 1, + mockedRead: 1, + mockedClose: 1 + }); + + delete proto._mockedBinding; + target.read(); + assert.deepEqual(calls, { + read: 2, + close: 1, + mockedRead: 1, + mockedClose: 1 + }); + target.close(); + assert.deepEqual(calls, { + read: 2, + close: 2, + mockedRead: 1, + mockedClose: 1 + }); + }); +}); From bef0a6cd064cbb7e873d044d410d4848bb3e1cc4 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 17 Sep 2021 12:17:53 +0200 Subject: [PATCH 4/5] readfilecontext: more tests for coverage + fix AbortError prototype --- lib/error.js | 1 + test/lib/readfilecontext.js | 41 +++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/lib/error.js b/lib/error.js index d4c3a1be..3619e688 100644 --- a/lib/error.js +++ b/lib/error.js @@ -50,6 +50,7 @@ function AbortError() { this.name = 'AbortError'; Error.captureStackTrace(this, AbortError); } +AbortError.prototype = new Error(); /** * FSError constructor. diff --git a/test/lib/readfilecontext.js b/test/lib/readfilecontext.js index a5bb5be1..f82475d4 100644 --- a/test/lib/readfilecontext.js +++ b/test/lib/readfilecontext.js @@ -1,12 +1,15 @@ 'use strict'; const helper = require('../helper'); +const fs = require('fs'); +const mock = require('../../lib/index'); const { patchReadFileContext, getReadFileContextPrototype } = require('../../lib/readfilecontext'); const assert = helper.assert; +const inVersion = helper.inVersion; describe('getReadFileContextPrototype', function() { it('provides access to the internal ReadFileContext', function() { @@ -105,3 +108,41 @@ describe('patchReadFileContext', function() { }); }); }); + +describe('fs.readFile() with ReadFileContext', function() { + // fs.readFile() is already tested elsewhere, here we just make sure we have + // coverage of the mocked ReadFileContext implementation. + + beforeEach(function() { + mock({ + 'path/to/file.txt': 'file content', + 1: 'fd content' + }); + }); + afterEach(mock.restore); + + inVersion('>=15.0.0').it('allows file reads to be aborted', function(done) { + const controller = new AbortController(); + const {signal} = controller; + + fs.readFile('path/to/file.txt', {signal}, function(err) { + assert.instanceOf(err, Error); + assert.equal(err.name, 'AbortError'); + assert.equal(err.code, 'ABORT_ERR'); + done(); + }); + + // By aborting after the call it will be handled by the context rather than readFile() + controller.abort(); + }); + + it('allows file reads with a numeric descriptor', function(done) { + // This isn't actually supported by mock-fs, but let's make sure the call goes through + // It also covers the case of reading an empty file and reading with encoding + fs.readFile(1, 'utf-8', function(err, data) { + assert.isNull(err); + assert.equal(data, ''); + done(); + }); + }); +}); From d7237ca3c7424fad6b72d922991b664c8d685e5c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 17 Sep 2021 13:19:24 +0200 Subject: [PATCH 5/5] readfilecontext: added test for reading file with unknown length --- lib/readfilecontext.js | 2 +- test/lib/readfilecontext.js | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/readfilecontext.js b/lib/readfilecontext.js index 699b6cbb..4bd66ee6 100644 --- a/lib/readfilecontext.js +++ b/lib/readfilecontext.js @@ -62,7 +62,7 @@ exports.patchReadFileContext = function(prototype) { bytesRead === kReadFileUnknownBufferLength ? context.buffer : context.buffer.slice(0, bytesRead); - Array.prototype.push.apply(context.buffers, buffer); + context.buffers.push(buffer); } context.read(); } diff --git a/test/lib/readfilecontext.js b/test/lib/readfilecontext.js index f82475d4..c95f93d2 100644 --- a/test/lib/readfilecontext.js +++ b/test/lib/readfilecontext.js @@ -1,5 +1,6 @@ 'use strict'; +const constants = require('constants'); const helper = require('../helper'); const fs = require('fs'); const mock = require('../../lib/index'); @@ -145,4 +146,29 @@ describe('fs.readFile() with ReadFileContext', function() { done(); }); }); + + it('allows file reads with unknown size', function(done) { + mock({ + 'unknown-size.txt': function() { + const file = mock.file({ + content: Buffer.from('unknown size') + })(); + + // Override getStats to drop the S_IFREG flag + const origGetStats = file.getStats; + file.getStats = function() { + const stats = origGetStats.apply(this, arguments); + stats[1] ^= constants.S_IFREG; + return stats; + }; + return file; + } + }); + + fs.readFile('unknown-size.txt', 'utf-8', function(err, data) { + assert.isNull(err); + assert.equal(data, 'unknown size'); + done(); + }); + }); });