Fix fs ENOENT error message #2669
Conversation
On commands with new_path, if the destination path doesn't exist, it now prints the error message correctly.
| char* path = req->path; | ||
| if (req->new_path && req->errorno == UV_ENOENT) { | ||
| uv_fs_t stat_req; | ||
| if (uv_fs_stat(Loop(), &stat_req, req->path, NULL) == 0) { |
There was a problem hiding this comment.
Sorry, but that doesn't look like the proper way to do it. It adds the overhead of an extra syscall and it's race-y: the file could have been renamed, moved or unlinked in the mean time.
There was a problem hiding this comment.
Thanks for the comment ;) I was trying not to do a second syscall but unfortunately, ENOENT doesn't really tell much about what failed.
Would it be acceptable if this check is done in eio_execute (in deps/uv/src/unix/eio.c), only when executing methods with two path parameters? It's still one syscall but not as race-y :)
I think on Windows it returns a different error code for each case but I don't have it to test.
There was a problem hiding this comment.
I also didn't know about this http://github.com/bnoordhuis/libuv
Should I be updating on that project instead?
There was a problem hiding this comment.
Would it be acceptable if this check is done in eio_execute (in deps/uv/src/unix/eio.c), only when executing methods with two path parameters? It's still one syscall but not as race-y :)
Diogo, I've discussed it with @piscisaureus and we both feel that it's not a good change. We won't take this patch but an alternative node.js-only change where the exception contains both the old and the new name would be acceptable.
I also didn't know about this http://github.com/bnoordhuis/libuv
Should I be updating on that project instead?
Hah no, that's my personal fork where I push work-in-progress things to. I'll update the description.
There was a problem hiding this comment.
That's ok ;) I'm sure I'll get one in at some point hehe
Regarding the node.js-only, just to make it clear, you mean the javascript side only right?
I need to investigate how exceptions work between both, that actually gave me an idea on where to look, might be completely wrong but I got to try hehe
I'll leave this request and changes on for now, as reference, but I'll revert them later :)
Cheers
There was a problem hiding this comment.
Regarding the node.js-only, just to make it clear, you mean the javascript side only right?
Yep, that's what I mean. Augmenting the exception in Node's C++ code would be acceptable too, but doing it from JS land is preferable.
I need to investigate how exceptions work between both, that actually gave me an idea on where to look, might be completely wrong but I got to try
Feel free to ping me on IRC if you want some pointers. I hang out as (unsurprisingly) bnoordhuis in #node.js on freenode.org.
There was a problem hiding this comment.
Going to leave this place now but will be back online soon ;) I usually go by DiogoSnows
Thanks a lot!
On commands with new_path, if the destination path doesn't exist,
it now prints the error message correctly.
It used to always print the source path even when it existed.
I basically added a check for which path caused the problem, in the
After(req)callback.Also added a
new_pathto theuv_fs_tstructure to keep track of both paths throughout the whole command execution (and after).I'm also calling the
uv_fs_stat(in theAftercallback) instead ofstatdirectly so that it uses the platform specific code, if any.This should be a very simple fix but was great to dig a bit around :)
I hope you like, let the questions/suggestions begin! :)
EDIT: Fixes Issue #685 (async only, I'll add the other later but it's a slightly different code path...)