fix: limit parallel file reads inside walk to 100 - #446
Conversation
walk optimistically opens a FD for every file and dir it can see. this hits EMFILE if you have a super deep and/or wide directory tree. we add a simple promise parallelism limiter that prevents more than 100 FDs from opening at once. Closes electron#248
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Working with Erick Zhao
Thanks for taking this on — bounding the individual fs operations (rather than the recursive walk calls) is the right shape, and the deep-tree deadlock test is a nice touch. But the limiter as written doesn't actually limit anything, and two of the new walk tests assert the unlimited behavior, so this needs another pass before it fixes #248.
The Test (22.12.x) CI failure is real and reproduces locally: ConcurrencyLimiter > limits promise parallelism to its passed limit fails with expected 50 to be 3. yarn lint also currently fails on src/util.ts (yarn lint:fix should sort out the formatting), and there are a few typos worth cleaning up while you're in there (numRunningPromsies, "lakge", "redccursive", "beacuse").
Details inline.
Generated by Claude Code
| } | ||
|
|
||
| async run<T>(promise: () => Promise<T>): Promise<T> { | ||
| if(this.numRunningPromsies > this.maxRunningPromises) { |
There was a problem hiding this comment.
numRunningPromsies is never incremented, so this condition is never true and the limiter is a no-op — every task runs immediately (this is why the new unit test fails with peak 50). You need to increment after acquiring a slot, and the comparison needs to be >= (with >, limit+1 tasks can run). One more subtlety: decrementing and then waking a waiter lets a brand-new run() call steal the slot before the waiter resumes, transiently exceeding the limit. Something like this passes both of your unit tests:
async run<T>(task: () => Promise<T>): Promise<T> {
if (this.numRunningPromises >= this.maxRunningPromises) {
await new Promise<void>((resolve) => this.waitingPromises.push(resolve));
}
this.numRunningPromises++;
try {
return await task();
} finally {
this.numRunningPromises--;
this.waitingPromises.shift()?.();
}
}|
|
||
| await walk(testWorkingDir); | ||
|
|
||
| expect(binaryCheck.peakOpenFiles).toBe(750); |
There was a problem hiding this comment.
This asserts unlimited concurrency (750 concurrent reads of 750 files) — it only passes because the limiter is currently a no-op, and it will fail once the limiter works. With 750 files and a working limiter this should saturate, so expect(binaryCheck.peakOpenFiles).toBe(MAX_OPEN_FILE_DESCRIPTORS) matches the test's name.
| const result = await walk(testWorkingDir); | ||
|
|
||
| expect(binaryCheck.calls).toBe(fileCount); | ||
| expect(binaryCheck.peakOpenFiles).toBeLessThanOrEqual(fileCount); |
There was a problem hiding this comment.
This is vacuous — peak can never exceed the file count. The test title promises "no more than 100", so this should be toBeLessThanOrEqual(MAX_OPEN_FILE_DESCRIPTORS) (already imported above).
| // Every single open call holds a file descriptor. and we quickly hit ulimit, | ||
| // thus throwing an EMFILE, if you have a very lakge set of files to sign. | ||
| // 100 is a very safe number of files to hold open concurrently. | ||
| // It's higher than basically every ulimit. |
There was a problem hiding this comment.
Inverted — 100 is safe because it's lower than basically every ulimit (macOS defaults to 256).
1 participant
walk optimistically opens a FD for every file and dir it can see. this hits EMFILE if you have a super deep and/or wide directory tree.
we add a simple promise parallelism limiter that prevents more than 100 FDs from opening at once.
Closes #248