tschaub / mock-fs

Configurable mock for the fs module
https://npmjs.org/package/mock-fs
Other
911 stars 86 forks source link

mock.restore() leaks (no garbage collection) #302

Closed nonara closed 4 years ago

nonara commented 4 years ago

Bug Report

Hello,

Unfortunately, it seems that garbage collection is not working after mock.restore().

In my case, I have a collection of files which are pre-loaded. The total size is around 100mb.

The format is:

{
  "/path/dir": {},
  "/path/dir/file.ext": "file content"
}

When I call mock.restore(), the expectation is that the memory usage would go down, however it does not.

Re-initializing mock() with the same object causes the memory to go up. Just a few calls to mock/restore puts me over 1.4gb very quickly. I discovered this because GH actions started failing due to exceeding oldspace size for Node 10.

Workaround

If anyone encounters the same issue, here is my workaround, which uses file caching that we can control

import { normalizeSlashes } from 'ts-node';
import shell from 'shelljs';
import path from 'path';
import fs from 'fs';
import FileSystem from 'mock-fs/lib/filesystem';
import { default as mock } from 'mock-fs';

/* ***************************************************** */
// Constants
/* ***************************************************** */
const cachedFiles = new Map<string, Buffer>();
const vanillaFiles = cacheDirectories(
  '/path/to/files', 
  '/path/to/otherfiles'
);

/* ***************************************************** */
// Helpers
/* ***************************************************** */
/**
 * Create a DirectoryItems from actual files and directories for mock-fs (uses caching)
 */
function cacheDirectories(...directory: string[]): FileSystem.DirectoryItems {
  const res: FileSystem.DirectoryItems = {};
  for (const dir of directory)
    for (const stat of shell.ls('-RAl', dir) as unknown as (fs.Stats & { name: string })[]) {
      const statPath = normalizeSlashes(path.join(dir, stat.name));
      res[statPath] =
        !stat.isFile() ? {} :
        <ReturnType<typeof mock.file>>(() => {
          const fileData = fs.readFileSync(statPath);
          cachedFiles.set(statPath, fileData);

          return Object.defineProperty(mock.file({ content: '' })(), '_content', {
            get: () => cachedFiles.get(statPath),
            set: (data: any) => cachedFiles.set(statPath, data)
          });
        });
    }

  return res;
}

/* ***************************************************** */
// Exports
/* ***************************************************** */
export function mockFs() {
  mock(vanillaFiles);
}

export function restoreFs() {
  mock.restore();
  cachedFiles.clear();
}

Feature Request

Access to original fs

It would be wonderful to have the ability to access the original fs module. Originally, I wanted to do a lazy load solution, which allowed me to only cache files when a readFileSync was called.

I'm mocking full node projects with node_modules (testing for https://npmjs.com/ts-patch), but many files never get loaded. It would be great to not have to load them.

It's easy to integrate with the above logic, however however, we'd need the ability to read file data from the actual filesystem. The only way I found to do that was by child_prcoess.spawnSync to create a new node instance and get the data from the output buffer. This was unfortunately too slow.

TLDR; Would love the have an fsOriginal accessible, or, minimally, readFileSync, so we could lazy-load and cache

3cp commented 4 years ago

I think I created that leak.

https://github.com/tschaub/mock-fs/blob/e42fdbb81c6da3232855369dce32863e6fa15774/lib/binding.js#L243-L266

It was to bypass some weird behaviour on nodejs v10+ when I did not understand what's going on. Then I vaguely remember that I figured it out in following work on other fixes. But I now forgot the details again 😅, this hack probably should be removed.

I will try to reclaim my lost memory, and provide you a build to test.

nonara commented 4 years ago

I think I created that leak.

Haha. No worries. I've been running into some extremely bizarre behaviour dealing with node / v8 lately, so I completely understand.

Since you're familiar with the architecture, what are you thoughts on the feasibility of providing a route toward reading a file outside of the mocked system?

If such a function was available, I'd be glad to do a PR, re-engineering my logic into a helper that allowed people to populate a FileSystem.DirectoryItems object from a set of real paths with the option to pre-load or lazyload files.

3cp commented 4 years ago

The similar feature was requested before, but nobody picked it up yet. #62, #213

3cp commented 4 years ago

Can you test locally against npm install tschaub/mock-fs#fix-leak?

nonara commented 4 years ago

@3cp Cool! Will have a look at it tomorrow. Meanwhile, I just wrapped up this pull request, which I think should help ease everyone's frustrations without doing any major changes to the core.

Let me know what you think.