fix: don't fail on closing fd after reset has been called (#550)

[adiktofsugar]

Spent a day tracking this down so I figured I could put in a fix. To review, the issue is that if you have a stream that happens to be open when you call reset, you get an error like this (assuming jest, but I'm sure it comes as a surprise to anyone on any testing platform):

> npx jest
Error: Unhandled error. (Error: EBADF: bad file descriptor, close
    at createError (/path/to/project/node_modules/memfs/src/node/util.ts:146:17)
    at Volume.getFileByFdOrThrow (/path/to/project/node_modules/memfs/src/volume.ts:540:33)
    at Volume.closeSync (/path/to/project/node_modules/memfs/src/volume.ts:818:23)
    at Immediate._onImmediate (/path/to/project/node_modules/memfs/src/volume.ts:578:25)
    at processImmediate (node:internal/timers:478:21)
    at process.topLevelDomainCallback (node:domain:160:15)
    at process.callbackTrampoline (node:internal/async_hooks:128:24) {
  code: 'EBADF'
})
    at Volume.ReadStream.emit (node:events:507:17)
    at Volume.ReadStream.emit (node:domain:488:12)
    at /path/to/project/node_modules/memfs/src/volume.ts:2397:18
    at Immediate._onImmediate (/path/to/project/node_modules/memfs/src/volume.ts:580:9)
    at processImmediate (node:internal/timers:478:21)
    at process.topLevelDomainCallback (node:domain:160:15)
    at process.callbackTrampoline (node:internal/async_hooks:128:24) {
  code: 'ERR_UNHANDLED_ERROR',
  context: Error: EBADF: bad file descriptor, close
      at createError (/path/to/project/node_modules/memfs/src/node/util.ts:146:17)
      at Volume.getFileByFdOrThrow (/path/to/project/node_modules/memfs/src/volume.ts:540:33)
      at Volume.closeSync (/path/to/project/node_modules/memfs/src/volume.ts:818:23)
      at Immediate._onImmediate (/path/to/project/node_modules/memfs/src/volume.ts:578:25)
      at processImmediate (node:internal/timers:478:21)
      at process.topLevelDomainCallback (node:domain:160:15)
      at process.callbackTrampoline (node:internal/async_hooks:128:24) {
    code: 'EBADF'
  }
}

I find it difficult to imagine someone calling reset and expecting an error when the setImmediate callback fires, so that's where I made my change. Other ways of fixing I thought about were:

• add a waitForFilesToClose, but potentially you're testing something where they wouldn't close? also it requires an extra command that you'd only use for this purpose.
• if you could somehow "reset" the actual file system, I'd imagine it would abort all the ongoing operations as well, so this seems ideologically correct (to me)

[G-Rath]

Can you confirm that this doesn't happen with the native fs? And if so, could you add a test too?

[adiktofsugar]

@G-Rath you can't reset the native fs, so I'm not sure what I'd be comparing to. The closest I can think of would be if the node process closed while a stream is open, but node keeps itself open in that case unless it detects a cycle. I can definitely add a test, though.

[adiktofsugar]

I thought maybe "reset" could be the same as "deleting all the files", so I tried to see what would happen if I deleted an open file and then ran close.:

const fd = fs.openSync(file);
fs.unlinkSync(file);
fs.closeSync(fd);

It doesn't throw an error. If you close it twice, though, it does (there's a test for that). So I decided to just do the getFileByFdOrThrow(fd, 'close'); step in the close function (synchronously) and then call closeFile() directly rather than closeSync. I think that's the most correct fix, but let me know if you don't agree.

[G-Rath]

thanks - I didn't realise reset was our own extra function rather than a Node one 🙂

[github-actions]

🎉 This PR is included in version 4.15.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀