ENOENT error message inconsistencies

[vsemozhetbyt]

• Version: 8.0.0-rc.0
• Platform: Windows 7 x64
• Subsystem: errors, fs, libuv, process

Currently, most of the ENOENT error messages have a similar signature:

Error: ENOENT: no such file or directory, [call] '/full/path/to/filename'

For example:

fs.accessSync('does-not-exist'); // etc...

Error: ENOENT: no such file or directory, access '...does-not-exist'
Error: ENOENT: no such file or directory, chmod '...does-not-exist'
Error: ENOENT: no such file or directory, link '...does-not-exist' -> '...does-not-exist2'
Error: ENOENT: no such file or directory, lstat '...does-not-exist'
Error: ENOENT: no such file or directory, open '...does-not-exist'
Error: ENOENT: no such file or directory, readlink '...does-not-exist'
Error: ENOENT: no such file or directory, rename '...does-not-exist' -> '...does-not-exist2'
Error: ENOENT: no such file or directory, rmdir '...does-not-exist'
Error: ENOENT: no such file or directory, scandir '...does-not-exist'
Error: ENOENT: no such file or directory, stat '...does-not-exist'
Error: ENOENT: no such file or directory, unlink '...does-not-exist'
Error: ENOENT: no such file or directory, utime '...does-not-exist'

I've stumbled upon 2 cases that have different and somehow confusing signatures:

1. Different order, no description, filename without quotes, no full path:

fs.watch('does-not-exist');
// Error: watch does-not-exist ENOENT

2. No file name / path at all (the call name can be wrongly considered as the file name):

process.chdir('does-not-exist');
// Error: ENOENT: no such file or directory, uv_chdir

Are these cases worth unification? Can they be addressed in Node.js or they are libuv features?

[targos]

I think it is worth unification.

[aastikta28]

Hi @targos ,

Is this being worked upon? Can I take it?

[joyeecheung]

@aastikta28 I don't think it's being actively worked on. To summary:

• Fixing process.chdir involves updating the ThrowUVException call in Chdir in src/node.cc to ThrowUVException(err, "chdir", nullptr, *path, nullptr). Ideally you can even update that to take a context and use errors.uvException to generate the errors in the JS land but that would be a bigger undertake.
• Fixing fs.watch would involve updating node_stat_watcher.cc to throw an error with the one generated by the C++ UVException function in src/node.cc, or better, updating it to take a context and throw the error in JS land using the JS counterpart errors.uvException instead of errnoException, this requires a bit more work. (EDIT: it's fs_event_wrap.cc for fs.watch, node_stat_watcher.cc for fs.watchFile, also, node_stat_watcher.cc doest not even handle errors..)

Either way the C++ side of things must be updated, so I am not sure if this is a good first issue, if you are comfortable with C++ and the bindings then it certainly could be. I guess the process.chdir one should be trivial enough though if you choose to only update the C++ code without migrating that to errors.uvException. If you want to take it, feel free to ping me in the PR for reviews or mentoring. I am working on migrating the errors in fs but have not reached this part yet, would be happy to get a helping hand. If no one takes it I believe I will eventually get to this part anyway.

[joyeecheung]

Ah, also just realized this is a breaking change since it changes the error messages that are likely to be matched by the users, I am not sure if there are any popular modules that special-case for these errors. The fixes would need CITGM runs.

[aastikta28]

@joyeecheung Thanks for the wonderful explanation. It looks way more complicated than I thought. But, I can definitely get started on the trivial part of just updating the c++ code for process.chdir without any migration. How can I get started on that?

[joyeecheung]

@aastikta28 You can check out the contributing guide on general guidance for building Node, committing and submitting PRs. To fix the error message, replace this:

node/src/node.cc

Line 1625 in feaf6ac

return env->ThrowUVException(err, "uv_chdir");

with env->ThrowUVException(err, "chdir", nullptr, *path, nullptr) should be enough, this would

1. Display the syscall as chdir instead of uv_chdir
2. Display the path in the erorr message

You can learn more about how the error message is constructed by reading the code of UVException in src/node.cc.

[SirR4T]

Would like to take a shot at this, if no one else's taking it up.

[joyeecheung]

@SirR4T Heads up: I am currently working on fs.watch in the last batch of fs errors migrations, but I think process.chdir has not been taken.

[joyeecheung]

BTW, I wrote a file name wrong in #12351 (comment) , to fix fs.watch, fs_event_wrap.cc needs to be changed instead of node_stat_watcher.cc because that one is for fs.watchFile (node_stat_watcher.cc does not throw any errors, also...it does not even check the error code returned from libuv! 😱I am going to fix that after #19089 lands to make it more consistent with fs.watch)

[SirR4T]

Cool, thanks! was about to look into fs.watch next. Would like to work on the below instead.

Ideally you can even update that to take a context and use errors.uvException to generate the errors in the JS land but that would be a bigger undertake.

Could you elaborate a bit here? or point to your changes in fs module, for reference?

[joyeecheung]

@SirR4T See #19089

[joyeecheung]

Also, forget about the second part of #12351 (comment), it turns out that the particular errors in fs.watch are actually pretty easy to fix if we don't change the error code (which is what grants a CITGM run really, and the current way it is we are not going to touch error.code in fs before v10 I believe). But fs.watch has a its own can of worms so I added a bunch more stuff in #19089

[SirR4T]

Hi @joyeecheung , still trying to understand what "take a context" would mean here. In #19089, I see that in C++ land, an FSEventWrap is unwrapped, processed on, and a return value of error is set, whenever an error occurs. This is then checked for in JS land. For process.chdir(), I couldn't find any JS land wrappers. Where should I be starting here?

[gireeshpunathil]

are there work still pending on this?

[SirR4T]

I can see that fs.watch and process.chdir have been fixed on master. Not sure if there are other inconsistencies, though. How would one go about checking that?

[vsemozhetbyt]

I cannot think of another way than just iterating over all appropriate fs methods. But there may not be any more issues if those were fixed)

[vsemozhetbyt]

I will close this issue for now, but let us know if you find any more inconsistencies.