Only works the first time

[bard]

Shown with promises and async/await for clarity. The issue doesn't change with nested callbacks, and makes it impossible to use mock-fs in automated tests.

const mockfs = require('mock-fs')
const fs = require('fs')
const util = require('util')
const readFileAsync = util.promisify(fs.readFile)

const test = async () => {
  try {
    mockfs({ foo: 'bar' })
    console.log('first run:', await readFileAsync('foo', 'utf8'))
    mockfs.restore()

    mockfs({ foo: 'bar' })
    console.log('second run', await readFileAsync('foo', 'utf8'))
    mockfs.restore()
  } catch (err) {
    console.error(err)
  }
}

test()

Output:

first run: bar
{ Error: EBADF, bad file descriptor
    at Binding._getDescriptorById (/tmp/test/node_modules/mock-fs/lib/binding.js:231:11)
    at Binding.<anonymous> (/tmp/test/node_modules/mock-fs/lib/binding.js:501:27)
    at maybeCallback (/tmp/test/node_modules/mock-fs/lib/binding.js:49:18)
    at Binding.read (/tmp/test/node_modules/mock-fs/lib/binding.js:500:10)
    at ReadFileContext.read (internal/fs/read_file_context.js:88:5)
    at FSReqWrap.readFileAfterStat [as oncomplete] (fs.js:271:11)
    at /tmp/test/node_modules/mock-fs/lib/binding.js:94:16
    at /tmp/test/node_modules/mock-fs/lib/binding.js:57:9
    at process._tickCallback (internal/process/next_tick.js:61:11)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
  message: 'EBADF, bad file descriptor',
  code: 'EBADF',
  errno: 9 }

Environment:

$ node -v
v10.10.0

[killmenot]

@bard got the same issue but after migrating to 10.11.0 it's gone

UPD: I was wrong, still doesn't work :(

[maxwellgerber]

Preliminary investigation and here's what I've got:

in Node 10 a new helper readFileContext utility was introduced - https://github.com/nodejs/node/blob/master/lib/fs.js#L285

This utility pulls functions directly from the binding, and doesn't reference the binding as an object with dynamic properties, making overriding it impossible.

https://github.com/nodejs/node/blob/master/lib/internal/fs/read_file_context.js#L4

It is also lazy-loaded, so the first time it's called it require's the current fs binding.

readFile makes a preliminary call to fs.open - which the first time in the test given is the open function created by the first instantiation of mockfs.

In the second call, it's still the first open function. So we end up opening a file descriptor in the wrong mocked FS and it blows up.

Side note: if you make a call to readFileAsync before ever calling mockFS, then no calls to the mocked readFileAsync work. The fact that it even works the first time is more of a fluke than anything else.

Maybe we could look into reusing the FileSystem and Binding objects created at https://github.com/tschaub/mock-fs/blob/master/lib/index.js#L59-L60 instead of recreating them on every call to mockFs? That wouldn't be a great fix - if any part of the test setup calls fs.read* it'll load the real FS and any code executed later won't be able to use the mock.

I'll try to submit a PR to https://github.com/nodejs/node to not dereference the binding functions in read_file_context.

TL;DR: It's borked for node 10. It could be less borked.

[maxwellgerber]

bad news - Node 11 is moving away from process.binding entirely
see: nodejs/node#22160
and the FS PR: nodejs/node#22478
So it looks like this library may end up being borked forever.

[mistic]

With the most recent changes on Node 10 and on Node 11 this library won't work anymore with logic it uses for apply the bindings. The only possible way right now seems to me like instead of binding the process.binding('fs') methods we need to reassign the methods for the require('fs') and then on restore also restore the original ones there. This also implies that a redesign for the methods we overload with this library. Any plans to do this @tschaub ?

[tschaub]

@mistic - The mock-fs@3 versions sort of worked this way (providing overrides for require('fs'). See #182 for the change that moved to overriding binding('fs') methods. The nice part about the binding is that there are many fewer methods to override. If someone wants to take a stab at overriding all fs methods instead, that might be worthwhile – I don't have any plans to take this on.

Ideally, we could engage with Node folks on ideas for an in-memory filesystem implementation. Providing an alternate binding felt like the most sensible way to do this. But it has been very painful to provide support across multiple Node versions.

Another thing that might be worth considering is dropping support for more than one version of Node. It would be far more straightforward to target a single major version. The reason I tried to support more than one version of Node was that I imagined that people wanted to run tests on multiple Node versions. Providing an in-memory filesystem implementation that works across multiple Node versions when there is no supported way to do such a thing has proven to be more work than I have time for.

[bard]

OP here. I'ts no drop-in replacement but I've been using https://github.com/streamich/memfs

[tschaub]

This should be addressed on the 4.8.0 release (thanks @huochunpeng, see #260).

[3cp]

I had a second look of this issue, which is the same cause for #264.

The reason for working for the first time is that first read is after first mock(), so
nodejs lib/fs.js

if (!ReadFileContext)
    ReadFileContext = require('internal/fs/read_file_context');

nodejs lib/internal/fs/read_file_context

const { FSReqWrap, close, read } = process.binding('fs');

trapped the first mocked close and read, not the original methods on fs binding. This is good. But it will fail after second mock, because it still use close and read of the first mocked binding.

For the case of #264, it trapped close and read of original methods on fs binding.