Free req.file.pathw in fs::ReadFileUtf8().

[Justin-Nietzel]

Always call uv_fs_req_cleanup after calling uv_fs_open instead of just when uv_fs_open returns a negative result. I referenced ReadFileSync from node:js2c when making this change.

https://github.com/bnoordhuis made the same suggestion based on the PR #49691.

Fixes: #57800

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66182/

[Justin-Nietzel]

I updated the commit comments after the first CI failure with module names in the title. I think it was mistaking some variable names as modules? I moved the original title into the description. Will that description also get flagged. Is there a safe way to format the variable name without triggering the module name CI check?

[tniessen]

@Justin-Nietzel You need to force-push to your branch issue-57800 with an amended commit message. Since req.file.pathw is an implementation detail in an external dependency, I'd suggest a commit message title such as fs: add missing call to uv_fs_req_cleanup or something like that.

Is it safe to also unconditionally call us_fs_req_cleanup(&req) in defer_close? It seems very odd to me that defer_close will always call uv_fs_req_cleanup(&req) while conditionally reusing &req for uv_fs_close. Before #49691, there was a seperate uv_fs_t for the close operation (cc @anonrig).

[Justin-Nietzel]

ReadFileSync in js2c.cc follows a very similar workflow to ReadFileUTF8 when using opening a file by file path. It re-uses the req object and always calls uv_fs_close after calling uv_fs_req_cleanup. I can't say with any certainty whether this behavior is "correct" or not, but I found it to be a useful reference.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (8dce721) to head (951437d).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57811      +/-   ##
==========================================
- Coverage   90.23%   90.23%   -0.01%     
==========================================
  Files         630      630              
  Lines      185465   185471       +6     
  Branches    36368    36374       +6     
==========================================
- Hits       167362   167353       -9     
+ Misses      10999    10996       -3     
- Partials     7104     7122      +18     

Files with missing lines	Coverage Δ
src/node_file.cc	76.72% <100.00%> (+<0.01%)	⬆️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcollina]

@Justin-Nietzel do you think you'd be able to add a test that verify the memory leak is fixed?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66194/

[Justin-Nietzel]

I probably could add a unit test. I would reference the readFileSync test and this worker thread memory test. I just had a few questions.

1. This leak scales with path length of the file bring read. I would probably want to make sure my test file has a path length of at least 100. Should I create a new test folder in fixtures for this issue so I'm not putting a really long file name directly in the fixtures folder? Alternatively, I could create the long file name in the temporary directory based on the current path instead.
2. For a noticable difference I would probably be doing 100,00 plus file reads. Should I put this in the pummel folder?
3. Based on my understanding of the documentation, each test is ran as a separate process. I don't have to worry about other tests effecting the RSS memory usage, right?

[Justin-Nietzel]

@mcollina I added a unit test for this fix and did my best to follow all the documentation I could find on style and testing. Let me know if you need me to make any adjustments.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66202/ 💛

[jasnell]

Landed in d0a5bd6...09ecd2e

[tniessen]

On a side note for future PRs, the second commit technically does not satisfy our requirements for commit messages since the leading verb is not imperative (added -> add).