EINVAL on Node 16.3.0

[sadasant]

Hello, team!

This is not a blocking issue for me, and it really could be just a bug on Node 16.3.0.

Using mock-fs v5 (and v4), the code I’ll share below works in Node 16.2.0, but does not work in 16.3.0.

The code:

const fs = require("fs");
const mockFs = require("mock-fs");

mockFs({
  "path/to/file": "value"
})

fs.readFile("path/to/file", { encoding: "utf8" }, (err, data) => console.log({ err, data }));

It logs the following on Node 16.2.0:

{  err: null, data: 'value' }

On Node 16.3.0, I get the following error:

{
  err: [Error: EINVAL: invalid argument, read] {
    errno: -22,
    code: 'EINVAL',
    syscall: 'read'
  },
  data: undefined
}

I’m thinking on opening an issue on Node’s source itself, but it seems to me that tracking down this to the underlying bug may take me a long time, so I’m making an issue here in case anybody can help me understand what is going on. (If anybody knows what’s up and wants to open the issue, please do so! But also share it here 🌞).

Any help is appreciated.

[3cp]

Nodejs v16.3.0 might just closed the door for mock-fs. https://github.com/nodejs/node/pull/38737/files

Now the internal/fs/read_file_context is statically loaded, means now mock-fs has no effect on the read_file_context required fs binding.

https://github.com/nodejs/node/blob/98139dfef1858b3879570d7edbd5afd3259c7de3/lib/internal/fs/read_file_context.js#L18

Our patch relies on the lazy loading of internal/fs/read_file_context in nodejs v10+, now there is no way to patch it.

mock-fs/lib/index.js

Lines 22 to 42 in 535a948

	/**
	* Pre-patch fs binding.
	* This allows mock-fs to work properly under nodejs v10+ readFile
	* As ReadFileContext nodejs v10+ implementation traps original binding methods:
	* const { FSReqWrap, close, read } = process.binding('fs');
	* Note this patch only solves issue for readFile, as the require of
	* ReadFileContext is delayed by readFile implementation.
	* if (!ReadFileContext) ReadFileContext = require('internal/fs/read_file_context')
	*
	* @param {string} key Property name.
	*/
	function patch(key) {
	const existingMethod = realBinding[key];
	realBinding[key] = function() {
	if (this._mockedBinding) {
	return this._mockedBinding[key].apply(this._mockedBinding, arguments);
	} else {
	return existingMethod.apply(this, arguments);
	}
	}.bind(realBinding);
	}

---

Note before Nodejs v10, Nodejs source code always uses const fsBinding = internalBinding('fs');, then use fsBinding.someMethod at runtime. So that was easy for mock-fs to patch the methods on fsBinding.

But now, Nodjes v10+ uses const { FSReqCallback, close, read } = internalBinding('fs');, that means the close and read methods ARE the original fsBinding method, there is no way for mock-fs to patch them after that line was evaluated when Nodejs loads up.

Previously the lazy loaded internal/fs/read_file_context gave us a small gap to patch before that read_file_context was loaded at runtime. Now the gap is gone :-(

[ryan-roemer]

First, mock-fs is just an absolute life-saver for fast, file I/O based tests. I can't imagine how cumbersome and slow testing life would be without it. Thank you to all the maintainers and contributors who have kept this project going in the face of Node.js internals changes over the years.

I just wanted to throw in that if we're truly at an unmockable place with Node.js internals, I'd personally be willing to consider a replacement for fs entirely in my library code like:

// Some external flag.
const IS_TEST = process.env.IS_TEST === "true";
const fs = IS_TEST ? require("fs") : require("mock-fs").alternateFsMock;

I normally don't like having test conditionals in actual library code, but for mock-fs if there's no other way, I'm fine with it 😉

[3cp]

@ryan-roemer you can use https://www.npmjs.com/package/memfs.

[sadasant]

Just an update: We moved to use fs and temp files. We're missing mock-fs, but we'll be ok 🌞

[Rugvip]

I'm also loving mock-fs and would hate to see it go :<

@3cp @tschaub how hacky would you be willing to get to patch things? The ReadFileContext prototype could be accessed for patching via this method:

const realBinding = process.binding('fs');

const originalOpen = realBinding.open;

realBinding.open = (_path, _flags, _mode, req) => {
  const proto = Object.getPrototypeOf(req.context)
  proto.read = () => { /* mock hook */ }
}
fs.readFile('/nothing.txt', () => {});

realBinding.open = originalOpen;

You'd then reroute the read and open methods to mock implementations when desired, and otherwise call the original ones, although I'm sure that all is simple enough at that point if this works 😁

[3cp]

@Rugvip have you tried your suggested method? It didn't work for me in either 16.5.0 or 14.17.0. The mocked proto.read is never called.

[3cp]

Just an update: We moved to use fs and temp files. We're missing mock-fs, but we'll be ok 🌞

@sadasant note if you run tests in Linux, you can use tmpfs to have a very fast "disk" in RAM.

[Rugvip]

@3cp Hmm, strange, could it be that it doesn't handle all different ways of reading files? Here's a more complete example, working with 14.17.0, 16.5.0, and 16.7.0:

const fs = require('fs');
const fsBinding = process.binding('fs');

const originalOpen = fsBinding.open;

fsBinding.open = (_path, _flags, _mode, req) => {
  const proto = Object.getPrototypeOf(req.context);

  console.log('Applying hook');
  proto.read = function () {
    console.log('Hook was called');
    this.callback(null, Buffer.from('hello'));
  };
};
fs.readFile('/nothing.txt', () => {});

fsBinding.open = originalOpen;

// Our actual call that we wanted to mock
fs.readFile('/', (err, content) => {
  console.log(`err=${err} content=${content}`);
});

[3cp]

Oh, thx! I misunderstood you code.