Skip to content

ENOENT error message inconsistencies #12351

Closed
Closed
ENOENT error message inconsistencies#12351
Labels
errorsIssues and PRs related to JavaScript errors originating in Node.js core.fsIssues and PRs related to file-system APIs and the fs module.processIssues and PRs related to the process subsystem.

Description

@vsemozhetbyt
vsemozhetbyt
opened on Apr 12, 2017
  • 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
  1. 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?

Activity

  1. vsemozhetbyt
    added
    errorsIssues and PRs related to JavaScript errors originating in Node.js core.
    fsIssues and PRs related to file-system APIs and the fs module.
    libuvIssues and PRs related to the libuv dependency or the uv binding.
    processIssues and PRs related to the process subsystem.
    on Apr 12, 2017
  2. cjihrig
    removed
    libuvIssues and PRs related to the libuv dependency or the uv binding.
    on May 17, 2017

  3. targos commented on Sep 3, 2017

    @targos
    targos
    on Sep 3, 2017
    Member

    I think it is worth unification.

  4. aastikta28 commented on Jan 4, 2018

    @aastikta28
    aastikta28
    on Jan 4, 2018

    Hi @targos ,

    Is this being worked upon? Can I take it?

  5. joyeecheung commented on Jan 4, 2018

    @joyeecheung
    joyeecheung
    on Jan 4, 2018
    Member

    @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.

  6. joyeecheung
    added
    mentor-available
    on Jan 4, 2018

  7. joyeecheung commented on Jan 4, 2018

    @joyeecheung
    joyeecheung
    on Jan 4, 2018
    Member

    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.

  8. aastikta28 commented on Jan 4, 2018

    @aastikta28
    aastikta28
    on Jan 4, 2018

    @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?

  9. joyeecheung commented on Jan 5, 2018

    @joyeecheung
    joyeecheung
    on Jan 5, 2018
    Member

    @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.

  10. SirR4T commented on Mar 2, 2018

    @SirR4T
    SirR4T
    on Mar 2, 2018

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

  11. joyeecheung commented on Mar 2, 2018

    @joyeecheung
    joyeecheung
    on Mar 2, 2018
    Member

    @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.

  12. joyeecheung commented on Mar 2, 2018

    @joyeecheung
    joyeecheung
    on Mar 2, 2018
    Member

    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)

  13. SirR4T commented on Mar 2, 2018

    @SirR4T
    SirR4T
    on Mar 2, 2018

    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?

  14. joyeecheung commented on Mar 2, 2018

    @joyeecheung
    joyeecheung
    on Mar 2, 2018
    Member

    @SirR4T See #19089

  15. joyeecheung commented on Mar 2, 2018

    @joyeecheung
    joyeecheung
    on Mar 2, 2018
    Member

    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

  16. SirR4T commented on Mar 3, 2018

    @SirR4T
    SirR4T
    on Mar 3, 2018

    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?

  17. gireeshpunathil commented on Apr 27, 2018

    @gireeshpunathil
    gireeshpunathil
    on Apr 27, 2018
    Member

    are there work still pending on this?

  18. SirR4T commented on Aug 3, 2018

    @SirR4T
    SirR4T
    on Aug 3, 2018

    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?

  19. vsemozhetbyt commented on Aug 3, 2018

    @vsemozhetbyt
    vsemozhetbyt
    on Aug 3, 2018
    ContributorAuthor

    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)

  20. vsemozhetbyt commented on Aug 3, 2018

    @vsemozhetbyt
    vsemozhetbyt
    on Aug 3, 2018
    ContributorAuthor

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

  21. vsemozhetbyt
    closed this as completedon Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    errorsIssues and PRs related to JavaScript errors originating in Node.js core.fsIssues and PRs related to file-system APIs and the fs module.processIssues and PRs related to the process subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions