Skip to content

test: ensure tmp directory cleanup in check-emfile-handling.js#19036

Merged
mdjermanovic merged 5 commits intoeslint:mainfrom
LiviaMedeiros:test-emfile-cleanup
Oct 21, 2024
Merged

test: ensure tmp directory cleanup in check-emfile-handling.js#19036
mdjermanovic merged 5 commits intoeslint:mainfrom
LiviaMedeiros:test-emfile-cleanup

Conversation

@LiviaMedeiros
Copy link
Copy Markdown
Contributor

@LiviaMedeiros LiviaMedeiros commented Oct 21, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: cleanup in test

What changes did you make? (Give an overview)

Remove the test directory (tmp/emfile-check) before and after the test.

Before, because it might contain files from previous runs. ulimit values aren't constant, so the actual amount of files might be bigger than the test generates.

After, because we shouldn't waste inodes (and space, depending on filesystem) on a directory with potentially 100000+ temp files.

Is there anything you'd like reviewers to focus on?

Noting that there's no cleanup in case of unexpected failure (from execSync() or even generateFiles()), which is intentional for easier manual debugging/reproducing.

@LiviaMedeiros LiviaMedeiros requested a review from a team as a code owner October 21, 2024 11:06
@eslint-github-bot eslint-github-bot Bot added the chore This change is not user-facing label Oct 21, 2024
@netlify
Copy link
Copy Markdown

netlify Bot commented Oct 21, 2024 •
edited
Loading

✅ Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 58583ca
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/671657ebe040b3000813218f

@mdjermanovic
Copy link
Copy Markdown
Member

mdjermanovic commented Oct 21, 2024 •
edited
Loading

In CI, this fails on ubuntu with:

✅ EMFILE error encountered: EMFILE: too many open files, open 'tmp/emfile-check/file_65516.js'
node:fs:1506
  const result = binding.readdir(
                         ^

Error: EMFILE: too many open files, scandir 'tmp/emfile-check'
    at readdirSync (node:fs:1506:26)
    at _rmdirSync (node:internal/fs/rimraf:252:29)
    at rimrafSync (node:internal/fs/rimraf:195:7)
    at Object.rmSync (node:fs:1263:10)
    at /home/runner/work/eslint/eslint/tools/check-emfile-handling.js:112:12
    at <anonymous> {
  errno: -24,
  code: 'EMFILE',
  syscall: 'scandir',
  path: 'tmp/emfile-check'
}

Node.js v20.18.0
Error: Process completed with exit code 1.

Looks like fs.rmSync itself runs into EMFILE?

@mdjermanovic
Copy link
Copy Markdown
Member

mdjermanovic commented Oct 21, 2024

Locally, on Windows, I'm getting a different error:

✅ EMFILE error encountered: EMFILE: too many open files, open 'C:\projects\eslint\tmp\emfile-check\file_8187.js'
node:fs:1216
  binding.rmdir(pathModule.toNamespacedPath(path));
          ^

Error: ENOTEMPTY: directory not empty, rmdir 'C:\projects\eslint\tmp\emfile-check'
    at Object.rmdirSync (node:fs:1216:11)
    at _rmdirSync (node:internal/fs/rimraf:260:21)
    at rimrafSync (node:internal/fs/rimraf:193:7)
    at Object.rmSync (node:fs:1265:10)
    at C:\projects\eslint\tools\check-emfile-handling.js:112:12
    at <anonymous> {
  errno: -4051,
  code: 'ENOTEMPTY',
  syscall: 'rmdir',
  path: 'C:\\projects\\eslint\\tmp\\emfile-check'
}

@LiviaMedeiros
Copy link
Copy Markdown
Contributor Author

LiviaMedeiros commented Oct 21, 2024

Interesting. I don't think it's correct for rmSync to fail from EMFILE if maxRetries wasn't reached yet, perhaps this should be fixed in the upstream.

@mdjermanovic
Copy link
Copy Markdown
Member

mdjermanovic commented Oct 21, 2024

It's still failing. If we want to remove the directory and the files after the test, perhaps we should wait for all readFile()s to settle before doing that?

@LiviaMedeiros
Copy link
Copy Markdown
Contributor Author

LiviaMedeiros commented Oct 21, 2024

Couldn't reproduce it locally... I'm not sure if we can robustly wait until file descriptors are freed after readFiles from JS realm, and maxRetries/retryDelay is supposed to be more correct workaround than arbitrary setTimeout.
Let's try with even bigger maxRetries, because if it fails again, it would pinpoint at it as upstream issue.

@LiviaMedeiros
Copy link
Copy Markdown
Contributor Author

LiviaMedeiros commented Oct 21, 2024

CI / Test (ubuntu-latest, 18.x) (pull_request) Failing after 2m

Even though 8192 retries with default retryDelay should keep it trying for 13 minutes.

Adding delay helps, I'll try to rewrite it with Promise.allSettled for more robustness.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Oct 21, 2024
mdjermanovic
mdjermanovic approved these changes Oct 21, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 7259627 into eslint:main Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees

No one assigned

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing

Projects

Triage
Status: Complete

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@LiviaMedeiros @mdjermanovic