BUG: fs.rename gives incorrect error message on error

[isaacs]

Try this:

mkdir -p ./path/that/exists/
touch ./path/that/exists/file

test.js

(function () {
  "use strict";

  var fs = require("fs");

  fs.rename("path/that/exists/file", "path/that/doesntexist/file", function (err) {
    console.log(err);
  });
}());

node test.js

{ message: 'ENOENT, No such file or directory \'path/that/exists/file\''
, stack: [Getter/Setter]
, errno: 2
, path: 'path/that/exists/file'
}

expected:

{ message: 'ENOENT, No such file or directory \'path/that/doesntexists/file\''
, stack: [Getter/Setter]
, errno: 2
, path: 'path/that/exists/file'
}

AJ ONeal

[ry]

this is difficult to resolve because ENOENT doesn't distinguish between the two errors:

• the first filename not existing and
• the second directory not existing

[isaacs]

Maybe we could change how ENOENT is handled in this case, and report both filenames?

[isaacs]

Same behavior can be seen in fs.symlink, or any other fs function that takes two paths.

[ry]

sure

[ry]

v0.5.10:

% node x.js 
{ [Error: ENOENT, No such file or directory 'path/that/exists/file'] errno: 32, code: 'ENOENT', path: 'path/that/exists/file' }

[DiogoNeves]

Is there any function, at the moment, that needs more than 2 paths as input?

I was looking at the code and I think I can add a char* new_path; to the uv_fs_s struct and update the methods that call WRAP_EIO macro. (probably more as well). I'll also search for any methods that take a second path and update the error messages.
Does that sound any reasonable?

Just started looking at the code so, this might be complete nonsense... if you like the idea let me know, I'll finish it and submit for review ;)

[isaacs]

@DiogoNeves Off the top of my head: rename, symlink, link. (Technically, "symlink" doesn't require two paths, since the linkdata argument doesn't have to exist yet.)

[DiogoNeves]

I know this doesn't mean much hehe but I'm excited with the possibility of my first contribution! :)
http://bit.ly/zheRy9
I'm just going to look at the windows code (don't have a windows machine to test it though...). I'll also have a quick look at the tests on this.

[DiogoNeves]

Actually, quick question.
I'm new to this, I'm on Mac OS X is there any way I can at least know if the deps/uv/src/win/fs.c will build. Any suggestions? What do you usually do in these cases?

Thanks

[DiogoNeves]

@ry now I know what you mean... the error returned when either file doesn't exist is exactly the same. My previous fix was stupid hehe :)
Possible two solutions are:
1 (not ideal and expensive) - manually check each directory and print the one that doesn't exist. Personally I don't like it, more expensive, complex and error prone.
2 - Simply print something along the lines:

{ [Error: ENOENT, Failed to rename 'path/that/exists/file' to 'path/that/doesntexist/file'] errno: 32, code: 'ENOENT', path: 'path/that/exists/file' }

I'm still investigating other possible solutions, I don't like 'impossible' things hehe and it's great to dig around the code.
Sorry for all my excitement on the previous 2 comments ;)

EDIT: And the extra easy and safe option goes to... Display this error message:

{ [Error: ENOENT, Failed to rename 'path/that/exists/file'] errno: 32, code: 'ENOENT', path: 'path/that/exists/file' }

What do you think?

[DiogoNeves]

Ok, after irc chatting with @bnoordhuis I tried to come up with a way to pass new_path around without creating yet another copy.
I'm really sorry if this is not the right place to discuss these things (or if I shouldn't have created a page on my fork) but here are my findings so far:
https://github.com/DiogoNeves/node/wiki/Ideas

Obviously I didn't start changing all that code ;) just want to discuss it as a possible solution.

EDIT: Sorry, @bnoordhuis I don't know when will I be online tomorrow so I shared right now ;)

[bnoordhuis]

@DiogoNeves: A patch should not touch libuv or libeio. Duplicate data makes me sad but I'll cope.

[DiogoNeves]

I think I took it more as a challenge, my bad ;)
Anyway, I'm running out of ideas right now so I should probably move on and look at another issue.
The only simple way I could see this being done is wrapping the callback (in the js side) on those calls and simply append the newPath.
Thanks

[trevnorris]

Testing against v0.8.15, the error message doesn't seem to have the same format:

{ [Error: ENOENT, rename '/tmp/happy'] errno: 34, code: 'ENOENT', path: '/tmp/happy' }

The message (and not sure why message isn't enumerable, or displays on console.log()) doesn't contain the same text. So what does that mean for this issue?

[bnoordhuis]

Semi-related to #4314 though the patch for that issue is targeted at v0.8. As a fix for this issue would require adding a property to the Error object, it's arguably an API change and not suitable for v0.8.

The message (and not sure why message isn't enumerable, or displays on console.log()) doesn't contain the same text.

That's because of a V8 change where Error properties are no longer enumerable. I can't find the commit but it's been like that for a while now.

[xixixao]

Would be great to fix, thought I am gonna jump out of the window before finding this issue....

[JoeDailey]

Can confirm this is a problem on linux. Geez, I just wanted to explode because of this missleading error. This needs to be fixed as soon as possible. Has there been any headway towards the actual implementation? Or rather, what is the status of a fix?

[trevnorris]

@bnoordhuis Here's a horribly ugly patch, and only fixes the sync part, but want your thoughts on the general approach:

diff --git a/src/node_file.cc b/src/node_file.cc
index 3b287ec..6e11d3e 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -288,8 +288,14 @@ struct fs_req_wrap {
                          __VA_ARGS__,                                         \
                          NULL);                                               \
   if (err < 0) {                                                              \
+    int err2 = 0;                                                             \
+    if (dest != NULL) {                                                       \
+      uv_fs_t stat_wrap;                                                      \
+      err2 = uv_fs_stat(env->event_loop(), &stat_wrap, dest, NULL);           \
+    }                                                                         \
     if (dest != NULL &&                                                       \
-        (err == UV_EEXIST ||                                                  \
+        (err2 < 0 ||                                                          \
+         err == UV_EEXIST ||                                                  \
          err == UV_ENOTEMPTY ||                                               \
          err == UV_EPERM)) {                                                  \
       return env->ThrowUVException(err, #func, "", dest);                     \

IMO once code reaches an error path performance is secondary. So the sync stat to check if the file exists shouldn't matter. Thoughts?

[bnoordhuis]

IMO once code reaches an error path performance is secondary.

I don't know if that's true. Take for example the way fs.statSync() is used in require(). #8189 is tangentially related to that.

[trevnorris]

@bnoordhuis fair enough. Other than stat'ing one of the files to see if it's there, do you have an idea how the error message could differentiate between which path to display in the error message?

[bnoordhuis]

Sadly, no. It's bad design of the UNIX API, really. (Heresy, I know.)

[trevnorris]

@bnoordhuis haha, okay. and would you consider it a bad idea to do a "quick" uv_fs_stat() to check which file doesn't exist?

[JoeDailey]

In the case of an error, knowing the error is more beneficial than a very low efficiency hit. Are there any objections to that statement?

Also, In the cases where this is a production error and the possibility of a file not existing at runtime is greater than none, developers will be checking for file existence anyways (avoiding the error being thrown in the first place). In other words, this patch would only effect performance during development.

[bnoordhuis]

Well, there's the performance issue to consider but another gotcha is the race window between the two system calls. If rename("foo", "bar") fails but bar gets deleted before you can stat it, then you get a bogus error. It's perhaps not very likely but still something to keep in mind.

[trevnorris]

@bnoordhuis Oy, and there you go pointing out the obvious but PITA edge cases. ;P

So, seems like it's technically impossible to be absolutely certain what file didn't exist at the time uv_fs_rename() was run. Maybe we should just report both the file and the path, say it's one of those two, and be done with this.

[JoeDailey]

Haha, that's no fun.

[dukeatcoding]

UP, really confusing bug

This bug took me some minutes, to figure out that the last subfolder in the destination was not present

[jasnell]

Ran into a similar problem a few years ago in a different system. The only solution we came up with at the time was to take the performance hit and acknowledge the inherent race condition in the documentation. The warning to developers was basically, "if it hurts when you do that, then don't do it." Improving the error report as @trevnorris suggested in comment 54381951 would be good.

[robertkowalski]

@piscisaureus is this still an issue in io.js?

[mhdawson]

Confirmed still exists on v0.12 for ubuntu

[mhdawson]

Confirmed issues exists in io.js as well

[piscisaureus]

Libuv (at least on windows, don't know about unix) can't tell whether an error is related to the 'source' or 'target' path. Therefore, the only option we have is to display both paths.

io.js does that:

C:\Users\Bert Belder>iojs
> fs.renameSync('foo', 'bar')
Error: ENOENT: no such file or directory, rename 'C:\Users\Bert Belder\foo' -> 'C:\Users\Bert Belder\bar'
    at Error (native)
    at Object.fs.renameSync (fs.js:685:18)
    at repl:1:4
    at REPLServer.defaultEval (repl.js:155:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:309:12)
    at emitOne (events.js:77:13)
    at REPLServer.emit (events.js:166:7)
    at REPLServer.Interface._onLine (readline.js:208:10)
>

(@mhdawson - you reported that this is still an issue in io.js, but in fact this was fixed late January in nodejs/node@bc2c85c. I suspect that you may have accidentally tested node or a really old io.js version)

[jasnell]

Ok, so the question that remains is: do we want to backport this fix into v0.12 or joyent/node master or pick it up when we move to the converged stream? (I'm inclined to wait until we moved to the converged stream)

[mhdawson]

I think I ran on a recent enough io.js but when we reviewed the message the initial thought was that it still was not clear enough. Given your explanation about not being able to tell which path is the problem I can see its the best we can do.

Since its a P3 it should be able to wait until the converged stream.

[jasnell]

Agreed on waiting for the converged stream.

[jasnell]

at this point, I'm inclined to close this here. If someone wishes to backport the io.js fix to v0.12 then so be it, but the priority is low.