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

process: improve error message for process.cwd() when directory is deleted #57053

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ankush1oo8
Copy link

@Ankush1oo8 Ankush1oo8 commented Feb 14, 2025

This PR improves the error message thrown by process.cwd() when the current working directory is deleted. Instead of throwing a new error, it enhances the original error message, making it clearer that the directory was deleted while the process was still inside it.

Changes Made:

  • Modified wrappedCwd() to enhance the existing error message for ENOENT: uv_cwd instead of creating a new Error instance.
  • Ensured the error message explicitly states the reason and how to resolve it (using process.chdir() to switch directories).

Before (Old Behavior):

When the working directory was deleted, process.cwd() threw a generic error with little context:

Error: ENOENT: no such file or directory, uv_cwd

After (New Behavior):

Now, the error provides a more meaningful message:

Error: Current working directory does not exist - this can happen if the directory was deleted while the process was still inside it. Original error: ENOENT: no such file or directory, uv_cwd

Relevant Issue:

(If this PR fixes an issue, add the issue number here)
Example: Fixes #XXXX

PR-URL:

#57053

Reviewer Notes:

  • Based on feedback from @gurgunday, I’ve updated the PR to modify the existing error message instead of creating a new error.
  • Please review and let me know if further refinements are needed!

…leted

If the current working directory is deleted, process.cwd() throws
a generic 'uv_cwd' error, which can be confusing. This commit
enhances the error message by modifying the original error instead
of throwing a new one.

PR-URL: nodejs#57053
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 14, 2025
@Qard
Copy link
Member

Qard commented Feb 16, 2025

Thanks for taking this one on! ❤️

A few things though:

Why would mutating the error be preferable to using error.cause? (For example:

return PromiseReject(new AbortError(undefined, { cause: signal.reason }));
) Question for @gurgunday I suppose on why modifying the error was suggested. Doing this would lose the stack information of the original error whereas error.cause would preserve it, which may be helpful.

This also needs a test. I think the change to the native function may make the javascript change unreachable due to no longer having the syscall info. 🤔

And lastly, a more minor detail: a couple lint checks fail as the commit message is too long and the message lines are too long. We limit lines to 80 columns.

Copy link

codecov bot commented Feb 16, 2025

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.11%. Comparing base (cc7018e) to head (6599712).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
...ernal/bootstrap/switches/does_own_process_state.js 58.33% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #57053   +/-   ##
=======================================
  Coverage   89.10%   89.11%           
=======================================
  Files         665      665           
  Lines      193203   193213   +10     
  Branches    37216    37216           
=======================================
+ Hits       172158   172181   +23     
- Misses      13768    13778   +10     
+ Partials     7277     7254   -23     
Files with missing lines Coverage Δ
src/node_process_methods.cc 85.82% <100.00%> (ø)
...ernal/bootstrap/switches/does_own_process_state.js 93.50% <58.33%> (-3.03%) ⬇️

... and 41 files with indirect coverage changes

@Ankush1oo8
Copy link
Author

what should further with this pr

@gurgunday
Copy link
Contributor

Why would mutating the error be preferable to using error.cause?

I agree that error.cause is better!

I believe originally there was a new error thrown without .cause and I had said that it would be better to throw the same error instead, but error.cause would contain even more information so I think it's the best way to do it

@Qard
Copy link
Member

Qard commented Feb 20, 2025

what should further with this pr

I think @jasnell has the best advice here--add the error as a THROW_ERR_... macro in node_errors.h and just do everything on the native side. No need for a JS side wrapping error at all if the original error from the native side has sufficient information included.

@Ankush1oo8
Copy link
Author

Please check the pr
I have made the necessary changes

@jasnell
Copy link
Member

jasnell commented Feb 21, 2025

Code change LGTM but this needs a test. The test might be a bit tricky but it could create a temporary directory with a script in it, run that script, delete that directory, then call the api from the script that was in that directory, etc.... anyway, once a test is added I'm happy to approve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants