Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Win 10 pro still getting EMFILE: too many open files error. #194

Closed
stevemarksd opened this issue Aug 4, 2020 · 24 comments · Fixed by #207
Closed

Win 10 pro still getting EMFILE: too many open files error. #194

stevemarksd opened this issue Aug 4, 2020 · 24 comments · Fixed by #207

Comments

@stevemarksd
Copy link

I've added this at the root of my app

var realFs = require('fs')
var gracefulFs = require('graceful-fs')
gracefulFs.gracefulify(realFs)

But I'm still getting EMFILE: too many open files error.

Any ideas what else can I do? Win 10 pro

@strarsis
Copy link

strarsis commented Nov 14, 2020

+1, same issue on WSL 2 on Windows 10.

@stevemarksd: I had to globally patch fs as some dependencies I use require fs on their own, and rarely offer a configuration option for what fs should be used.

@AndreasWJ
Copy link

Have the same issue as @stevemarksd

@strarsis , what do you mean with "globally patch fs"? Is it the same as calling gracefulFs.gracefulify(...); in your app.js?

@strarsis
Copy link

@AndreasWJ: No, it isn't the same. The global patch approach will replace all uses of fs. Some libraries offer a separate key in the option object, e.g. fs, others don't have any support for a specific fs object, hence you have to patch fs globally.

@AndreasWJ
Copy link

@AndreasWJ: No, it isn't the same. The global patch approach will replace all uses of fs. Some libraries offer a separate key in the option object, e.g. fs, others don't have any support for a specific fs object, hence you have to patch fs globally.

Thanks for clearing it up!

I've patched it globally through the code below, but it still doesn't work. How did you solve it?

const Module = require('module');
const gracefulFs = require('graceful-fs');

module.exports = () => {
	Module.prototype.require = new Proxy(Module.prototype.require, {
		// Apply is used to trap function calls
		// thisArg, the value of 'this' in the caller's context
		// argumentsList = third parameter
		apply(target, thisArg, [moduleName, ...otherArgs]) {
			if (/fs/g.test(moduleName)) {
				console.log('Detected fs import from', thisArg);
				// Replace the required 'fs' module with 'graceful-fs'
				const fs = Reflect.apply(target, thisArg, [moduleName, ...otherArgs]);
				gracefulFs.gracefulify(fs);
				return fs;
			}

			// After replacing fs, return Reflect.apply(...) that gives accessors
			// the original behavior with the changes made above
			return Reflect.apply(target, thisArg, [moduleName, ...otherArgs]);
		},
	});
};

Sorry for the inconvience. I'm fairly new to Javascript

@strarsis
Copy link

You should do this in top level of your app and as soon as possible
(see https://github.com/isaacs/node-graceful-fs#global-patching):

var realFs = require('fs')
var gracefulFs = require('graceful-fs')
gracefulFs.gracefulify(realFs)

You use new Proxy and such, but when this module is required too late, fs invocations already occure and cause issues.
What modules are you requiring by the way? Maybe they offer a dedicated fs option?

@AndreasWJ
Copy link

You should do this in top level of your app and as soon as possible
(see https://github.com/isaacs/node-graceful-fs#global-patching):

var realFs = require('fs')
var gracefulFs = require('graceful-fs')
gracefulFs.gracefulify(realFs)

You use new Proxy and such, but when this module is required too late, fs invocations already occure and cause issues.
What modules are you requiring by the way? Maybe they offer a dedicated fs option?

I put the snippet first in my index.js, however, it throws the same error.

I use the module called 'vpk'(link) which extracts thousands of game files from Valve games. In the process of extracting these I'm reading thousands of files, hence the error being thrown. Unfortunantely, the module is kind of niche and it doesn't support a fs option... Maybe its worth to fork the module and add it myself?

@strarsis
Copy link

vsp internally uses JBinary which internally also requires fs, so its fs would need to be patched, too.
Can you see where exactly the file errors come from? Is there a stack trace?

@AndreasWJ
Copy link

AndreasWJ commented Dec 29, 2020

vsp internally uses JBinary which internally also requires fs, so its fs would need to be patched, too.
Can you see where exactly the file errors come from? Is there a stack trace?

I really appreciate the help! I'm clueless with these things. Here's the stack trace:

Extracted file with content <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 01 00 00 00 00 c0 08 06 00 00 00 eb ed bd 66 00 00 c2 50 49 44 41 54 78 9c ec fd 75 bc 65 57 7d ... 49751 more bytes>
node:fs:495
  handleErrorFromBinding(ctx);
  ^

Error: EMFILE: too many open files, open 'data/resource/flash/econ/stickers/cologne2015/sig_kennys_gold_large.png'
    at Object.openSync (node:fs:495:3)
    at Object.writeFileSync (node:fs:1519:35)
    at ExtractSticker.extractFile (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\Handler.js:107:7)
    at ExtractSticker.execute (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\ExtractSticker.js:43:9)
    at ExtractSkin.executeNext (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\Handler.js:54:20)
    at ExtractSkin.execute (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\ExtractSkin.js:45:16)
    at ExtractWeapon.executeNext (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\Handler.js:54:20)
    at ExtractWeapon.execute (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\ExtractWeapon.js:51:16)
    at SteamAssetExtractor.extractAssets (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetExtractor.js:104:17)
    at new SteamAssetExtractor (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetExtractor.js:27:8)
    at SteamAssetDownloader.<anonymous> (C:\Users\known\Desktop\ChatChops\alert\src\index.js:59:25)
    at SteamAssetDownloader.emit (node:events:376:20)
    at SteamAssetDownloader.set ready [as ready] (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetDownloader.js:33:9)
    at SteamAssetDownloader.update (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetDownloader.js:132:14) {
  errno: -4066,
  syscall: 'open',
  code: 'EMFILE',
  path: 'data/resource/flash/econ/stickers/cologne2015/sig_kennys_gold_large.png'
}

For reference, here is the line of code supposedly throwing the error:

console.log('Extracted file with content', data);
fs.writeFileSync(finalPath, data);
console.log(`File written to: ${finalPath}`);

I tried removing the specific file that's involved when the error is thrown; 'data/resource/flash/econ/stickers/cologne2015/sig_kennys_gold_large.png'. Just to rule it out, and it made no difference.

Is it impossible to "switch out requires" in JBinary before loading requiring the vpk module? Essentially what I've done so far with my Proxy solution. The idea was to replace all imports like require('fs'); to require('graceful-fs');, is there no other way to precede the fs invocations?

@strarsis
Copy link

strarsis commented Dec 29, 2020

So C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\Handler.js:107:7 doesn't seem to be a library but your own application code. What does it call on that line? Just plain fs or a library (vpk)?

@AndreasWJ
Copy link

AndreasWJ commented Dec 30, 2020

So C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\Handler.js:107:7 doesn't seem to be a library but your own application code. What does it call on that line? Just plain fs or a library (vpk)?

Plain fs, fs.writeFileSync(...) to be exact.

Here is Handler:105-Handler:109:

if (Handler.validateFilePath(finalPath)) {
	console.log('Extracted file with content', data);
	fs.writeFileSync(finalPath, data);
	console.log(`File written to: ${finalPath}`);
}

Where finalPath is the path I'm writing the file to. And data is file data which is sourced from the aforementioned vpk module by calling vpkInstance.getFile(...). Not that it should make any difference where the data is sourced from.

@strarsis
Copy link

@AndreasWJ: But when the graceful-fs patch is applied, that fs invocation shouldn't cause unhandled EMFILE exceptions.
For diagnosis, could you patch just this one particular fs instance with graceful-fs and check whether the exception at exactly that line still occur?
What node version are you using? Could you try to use a recent LTS?

@AndreasWJ
Copy link

@strarsis: I've "globally" patched fs in my index.js, the first line that's executed is the patch:

const realFs = require('fs');
// eslint-disable-next-line import/newline-after-import
const gracefulFs = require('graceful-fs');
gracefulFs.gracefulify(realFs);
const http = require('http');
const socketio = require('socket.io');
more requires and business logic...

I've manually patched the fs import in Handler.js as well:

const Promise = require('bluebird');
const fs = Promise.promisifyAll(require('graceful-fs'));
...

However, weirdly enough the error is thrown from exactly the same line of code like before; Handler:107. Here's the stack trace:

Extracted file with content <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 01 00 00 00 00 c0 08 06 00 00 00 eb ed bd 66 00 00 c2 50 49 44 41 54 78 9c ec fd 75 bc 65 57 7d ... 49751 more bytes>
node:fs:495
  handleErrorFromBinding(ctx);
  ^

Error: EMFILE: too many open files, open 'data/resource/flash/econ/stickers/cologne2015/sig_kennys_gold_large.png'
    at Object.openSync (node:fs:495:3)
    at Object.writeFileSync (node:fs:1519:35)
    at ExtractSticker.extractFile (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\Handler.js:107:7)
    at ExtractSticker.execute (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\ExtractSticker.js:43:9)
    at SteamAssetExtractor.extractAssets (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetExtractor.js:104:17)
    at new SteamAssetExtractor (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetExtractor.js:27:8)
    at SteamAssetDownloader.<anonymous> (C:\Users\known\Desktop\ChatChops\alert\src\index.js:59:25)
    at SteamAssetDownloader.emit (node:events:376:20)
    at SteamAssetDownloader.set ready [as ready] (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetDownloader.js:33:9)
    at SteamAssetDownloader.update (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetDownloader.js:132:14) {
  errno: -4066,
  syscall: 'open',
  code: 'EMFILE',
  path: 'data/resource/flash/econ/stickers/cologne2015/sig_kennys_gold_large.png'
}
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! alert@1.0.0 start: `node src/index.js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the alert@1.0.0 start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\known\AppData\Roaming\npm-cache\_logs\2020-12-30T15_49_41_183Z-debug.log

@AndreasWJ
Copy link

AndreasWJ commented Dec 30, 2020

@strarsis: As for node version I'm using v15.4.0.
I'm going to try the latest LTS to see if it makes any difference!

Edit: Downloaded the latest LTS; v14.15.3. For some reason I had an experimental version before, no idea how.
Anyways, the error persists:

Extracted file with content <Buffer 89 50 4e 47 0d 0a 1a 0a 00 00 00 0d 49 48 44 52 00 00 01 00 00 00 00 c0 08 06 00 00 00 eb ed bd 66 00 00 c2 50 49 44 41 54 78 9c ec fd 75 bc 65 57 7d ... 49751 more bytes>
(node:10372) UnhandledPromiseRejectionWarning: Error: EMFILE: too many open files, open 'data/resource/flash/econ/stickers/cologne2015/sig_kennys_gold_large.png'
    at Object.openSync (fs.js:476:3)
    at Object.writeFileSync (fs.js:1467:35)
    at ExtractSticker.extractFile (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\Handler.js:107:7)
    at ExtractSticker.execute (C:\Users\known\Desktop\ChatChops\alert\src\services\Extractors\ExtractSticker.js:43:9)
    at SteamAssetExtractor.extractAssets (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetExtractor.js:104:17)
    at new SteamAssetExtractor (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetExtractor.js:27:8)
    at SteamAssetDownloader.<anonymous> (C:\Users\known\Desktop\ChatChops\alert\src\index.js:59:25)
    at SteamAssetDownloader.emit (events.js:315:20)
    at SteamAssetDownloader.set ready [as ready] (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetDownloader.js:33:9)
    at SteamAssetDownloader.update (C:\Users\known\Desktop\ChatChops\alert\src\services\SteamAssetDownloader.js:132:14)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:10372) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:10372) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

@strarsis
Copy link

strarsis commented Dec 30, 2020

@AndreasWJ: This is strange, but it looks as that line still doesn't use the graceful-fs wrapper library, but rather plain fs instead.
No graceful-fs mentioned in the stack trace either.
Are you using WSL 1 or 2? Could it be an I/O issue, that WSL doesn't pick up file changes?
Are you using the latest release of graceful-fs? An adjustment had been made to it for making it work correctly with promisify:
1139b6f#diff-f740ecac46b2fdaa68156b133262813aa6f66218b11d8709bab83580e76e486dR134
Hm, you are using bluebird for promisification of graceful-fs/fs. Try to use util.promisify instead (you may have to promisify each used method) - does then the error go away for that particular line?

@AndreasWJ
Copy link

AndreasWJ commented Dec 30, 2020

@strarsis: I couldn't tell you what WSL I'm on, I have the standard 64 bit Windows 10 installer if that helps 😅.
In terms of graceful-fs I'm using version 4.2.4, the latest.

I completely removed bluebird, it was a leftover from another file, unneeded in Handler.js as there are no "Async" suffixes. And it is good to rule out... Yet the same error persists, and the same line.

I do agree, I have no idea why graceful-fs is left out of the stack trace. I'm going to do some investigative work and write back If I find anything. Again, thank you so much! I would never got this far without the help!

@AndreasWJ
Copy link

Might have found out why graceful-fs isn't working when globally patched. What I can gather from its source, graceful-fs doesn't cover any native fs functions that have the suffix sync, for example; fs.writeFileSync(...) is not handled by graceful-fs.

Therefore, there's another problem. I've successfully globally patched graceful-fs as a replacement to fs in my imported third-party modules. However, my third-party modules use functions with sync suffixes.

@strarsis: Is this something you're aware of? Or am I completely lost?

@medikoo
Copy link

medikoo commented Mar 4, 2021

As I investigated error is thrown by fs.openSync which is not patched by graceful-fs, and there seems no way it can work without patching as by looking at the source code of fs.openSync the error comes from native binding.

function openSync(path, flags, mode) {
  path = getValidatedPath(path);
  const flagsNumber = stringToFlags(flags);
  mode = parseFileMode(mode, 'mode', 0o666);

  const ctx = { path };
  const result = binding.open(pathModule.toNamespacedPath(path),
                              flagsNumber, mode,
                              undefined, ctx);
  handleErrorFromBinding(ctx);
  return result;
}

So it looks as clear bug on graceful-fs side. @isaacs ?

@coreyfarrell
Copy link
Collaborator

I'm almost positive it is impossible to 'fix' an EMFILE error given by sync functions. EMFILE means that the current process is out of FD's. It's impossible for the current process to close any existing FD's while openSync is running so a retry cycle can never work.

ENFILE is a bit different, in this case we could have a loop that wastes 100% of CPU cycles in the hope that another process closes some FD's, but that is a really bad/wasteful idea which I don't think is open for consideration.

Not having retry cycles to handle FD exhaustion from Sync functions is not a code bug, though I'll leave this issue open as the documentation can probably be improved to make this clear. My advice if you are facing this problem you need to avoid sync functions, or determine why your system is overloaded.

@medikoo
Copy link

medikoo commented Mar 4, 2021

I'm almost positive it is impossible to 'fix' an EMFILE error given by sync functions. EMFILE means that the current process is out of FD's. It's impossible for the current process to close any existing FD's while openSync is running so a retry cycle can never work.

@coreyfarrell indeed, that's a very good point. Still situation could be improved, and make such crashes less likely, if for async function we try to keep taken descriptors few counts below the maximum allowed. Then there's always some room for eventual sync processors, which come to play (also in most circumstances sync processors release taken descriptor immediately).

Of course there's no perfect solution to that. First we need to reach the limit, to know what's the process limit, and only afterwards we can workout the room for eventual sync processors.

@medikoo
Copy link

medikoo commented Mar 4, 2021

e.g. I was also using this solution in a past, which after limit is reached, simply keeps usage below the limit (without falling into EMFILE errors), this one could be relatively easily improved to leave room for sync processors

@coreyfarrell
Copy link
Collaborator

@medikoo I appreciate you trying but we're not open to making this module attempt to reserve FD's. This module is VERY heavily used and even the simplest changes result in regressions, often for other widely used packages. What you are proposing would be breaking and a very wide-reaching change. Further making this error less likely would only give a false security and encourage skipping necessary error checking.

@medikoo
Copy link

medikoo commented Mar 5, 2021

even the simplest changes result in regressions, often for other widely used packages

Do you mean regressions caused by eventual not intentional bugs introduced. Or do you envision the scenario where keeping used FD's few counts below the limit (with proper implementation) can break users applications?

Anyway such change can always be proposed with major version bump, and I agree that this comes as significant change that deserves a major bump.

I also think it can only work with globally applied patch, so it could be specific to it, and even could be behind the option.

@strarsis
Copy link

strarsis commented Mar 5, 2021

@AndreasWJ: Just was notified about this issue again. Yes, it appears that the sync methods are not patched.
You could ask the author of that library to get rid of sync methods (which aren't recommended anyway).
Or fork and modify the parts yourself (e.g. with some awaits).

@AndreasWJ
Copy link

@AndreasWJ: Just was notified about this issue again. Yes, it appears that the sync methods are not patched.
You could ask the author of that library to get rid of sync methods (which aren't recommended anyway).
Or fork and modify the parts yourself (e.g. with some awaits).

Thanks for the reply @strarsis. Will fork and modify needed modules myself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants