fs: improve error messages

* Include a description for the error message
* For rename, link, and symlink, include both the source and destination
  path in the error message.
* Expose the destination path as the `dest` property on the error object.
* Fix a bug where `ThrowUVException()` would incorrectly delegate to
  `Environment::TrowErrnoException()`.

API impact:
* Adds an extra overload for node::UVException() which takes 6
  arguments.

PR: https://github.com/iojs/io.js/pull/675
Fixes: https://github.com/iojs/io.js/issues/207
Closes: https://github.com/iojs/io.js/pull/293
Reviewed-by: Ben Noordhuis <info@bnoordhuis.nl>

diff --git a/src/env-inl.h b/src/env-inl.h
index c5f2328e1259ea7a36f1a8a20b245fb40d8e3489..46267ebd57dce9777a1168928c3c8e02c31fbd13 100644 (file)
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -377,9 +377,10 @@ inline void Environment::ThrowErrnoException(int errorno,
 inline void Environment::ThrowUVException(int errorno,
                                           const char* syscall,
                                           const char* message,
-                                          const char* path) {
+                                          const char* path,
+                                          const char* dest) {
   isolate()->ThrowException(
-      UVException(isolate(), errorno, syscall, message, path));
+      UVException(isolate(), errorno, syscall, message, path, dest));
 }
 
 inline v8::Local<v8::FunctionTemplate>

diff --git a/src/env.h b/src/env.h
index 24bb0905bc3f141dbe46cddc499fc51f491f0b1c..8dc9c968835ab5b29de11ca4ad6c4f2a9525a47e 100644 (file)
--- a/src/env.h
+++ b/src/env.h
@@ -61,6 +61,7 @@ namespace node {
   V(cwd_string, "cwd")                                                        \
   V(debug_port_string, "debugPort")                                           \
   V(debug_string, "debug")                                                    \
+  V(dest_string, "dest")                                                      \
   V(detached_string, "detached")                                              \
   V(dev_string, "dev")                                                        \
   V(disposed_string, "_disposed")                                             \
@@ -407,7 +408,8 @@ class Environment {
   inline void ThrowUVException(int errorno,
                                const char* syscall = nullptr,
                                const char* message = nullptr,
-                               const char* path = nullptr);
+                               const char* path = nullptr,
+                               const char* dest = nullptr);
 
   // Convenience methods for contextify
   inline static void ThrowError(v8::Isolate* isolate, const char* errmsg);

diff --git a/src/node.cc b/src/node.cc
index 463b69936afd88285b81f19260efc7127d22fe12..7c9032bf02945b452d5e69c6f80e407cf14d0c0d 100644 (file)
--- a/src/node.cc
+++ b/src/node.cc
@@ -698,11 +698,10 @@ void ThrowUVException(v8::Isolate* isolate,
                       int errorno,
                       const char* syscall,
                       const char* message,
-                      const char* path) {
-  Environment::GetCurrent(isolate)->ThrowErrnoException(errorno,
-                                                        syscall,
-                                                        message,
-                                                        path);
+                      const char* path,
+                      const char* dest) {
+  Environment::GetCurrent(isolate)
+      ->ThrowUVException(errorno, syscall, message, path, dest);
 }
 
 
@@ -752,64 +751,78 @@ Local<Value> ErrnoException(Isolate* isolate,
 }
 
 
-// hack alert! copy of ErrnoException, tuned for uv errors
+static Local<String> StringFromPath(Isolate* isolate, const char* path) {
+#ifdef _WIN32
+  if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) {
+    return String::Concat(FIXED_ONE_BYTE_STRING(isolate, "\\\\"),
+                          String::NewFromUtf8(isolate, path + 8));
+  } else if (strncmp(path, "\\\\?\\", 4) == 0) {
+    return String::NewFromUtf8(isolate, path + 4);
+  }
+#endif
+
+  return String::NewFromUtf8(isolate, path);
+}
+
+
+Local<Value> UVException(Isolate* isolate,
+                         int errorno,
+                         const char* syscall,
+                         const char* msg,
+                         const char* path) {
+  return UVException(isolate, errorno, syscall, msg, path, nullptr);
+}
+
+
 Local<Value> UVException(Isolate* isolate,
                          int errorno,
-                         const char *syscall,
-                         const char *msg,
-                         const char *path) {
+                         const char* syscall,
+                         const char* msg,
+                         const char* path,
+                         const char* dest) {
   Environment* env = Environment::GetCurrent(isolate);
 
   if (!msg || !msg[0])
     msg = uv_strerror(errorno);
 
-  Local<String> estring = OneByteString(env->isolate(), uv_err_name(errorno));
-  Local<String> message = OneByteString(env->isolate(), msg);
-  Local<String> cons1 =
-      String::Concat(estring, FIXED_ONE_BYTE_STRING(env->isolate(), ", "));
-  Local<String> cons2 = String::Concat(cons1, message);
-
-  Local<Value> e;
+  Local<String> js_code = OneByteString(isolate, uv_err_name(errorno));
+  Local<String> js_syscall = OneByteString(isolate, syscall);
+  Local<String> js_path;
+  Local<String> js_dest;
 
-  Local<String> path_str;
+  Local<String> js_msg = js_code;
+  js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, ": "));
+  js_msg = String::Concat(js_msg, OneByteString(isolate, msg));
+  js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, ", "));
+  js_msg = String::Concat(js_msg, js_syscall);
 
-  if (path) {
-#ifdef _WIN32
-    if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) {
-      path_str = String::Concat(FIXED_ONE_BYTE_STRING(env->isolate(), "\\\\"),
-                                String::NewFromUtf8(env->isolate(), path + 8));
-    } else if (strncmp(path, "\\\\?\\", 4) == 0) {
-      path_str = String::NewFromUtf8(env->isolate(), path + 4);
-    } else {
-      path_str = String::NewFromUtf8(env->isolate(), path);
-    }
-#else
-    path_str = String::NewFromUtf8(env->isolate(), path);
-#endif
+  if (path != nullptr) {
+    js_path = StringFromPath(isolate, path);
 
-    Local<String> cons3 =
-        String::Concat(cons2, FIXED_ONE_BYTE_STRING(env->isolate(), " '"));
-    Local<String> cons4 =
-        String::Concat(cons3, path_str);
-    Local<String> cons5 =
-        String::Concat(cons4, FIXED_ONE_BYTE_STRING(env->isolate(), "'"));
-    e = Exception::Error(cons5);
-  } else {
-    e = Exception::Error(cons2);
+    js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, " '"));
+    js_msg = String::Concat(js_msg, js_path);
+    js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, "'"));
   }
 
-  Local<Object> obj = e->ToObject(env->isolate());
-  // TODO(piscisaureus) errno should probably go
-  obj->Set(env->errno_string(), Integer::New(env->isolate(), errorno));
-  obj->Set(env->code_string(), estring);
+  if (dest != nullptr) {
+    js_dest = StringFromPath(isolate, dest);
 
-  if (path != nullptr) {
-    obj->Set(env->path_string(), path_str);
+    js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, " -> '"));
+    js_msg = String::Concat(js_msg, js_dest);
+    js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, "'"));
   }
 
-  if (syscall != nullptr) {
-    obj->Set(env->syscall_string(), OneByteString(env->isolate(), syscall));
-  }
+  Local<Object> e = Exception::Error(js_msg)->ToObject(isolate);
+
+  // TODO(piscisaureus) errno should probably go; the user has no way of
+  // knowing which uv errno value maps to which error.
+  e->Set(env->errno_string(), Integer::New(isolate, errorno));
+  e->Set(env->code_string(), js_code);
+  e->Set(env->syscall_string(), js_syscall);
+  if (!js_path.IsEmpty())
+    e->Set(env->path_string(), js_path);
+  if (!js_dest.IsEmpty())
+    e->Set(env->dest_string(), js_dest);
 
   return e;
 }

diff --git a/src/node.h b/src/node.h
index 7d80dc35e0bb04315afab7934d17c43446a51694..097441e107009cc4d517283eafafe74b9bf2aa44 100644 (file)
--- a/src/node.h
+++ b/src/node.h
@@ -59,6 +59,12 @@ NODE_EXTERN v8::Local<v8::Value> UVException(v8::Isolate* isolate,
                                              const char* syscall = NULL,
                                              const char* message = NULL,
                                              const char* path = NULL);
+NODE_EXTERN v8::Local<v8::Value> UVException(v8::Isolate* isolate,
+                                             int errorno,
+                                             const char* syscall,
+                                             const char* message,
+                                             const char* path,
+                                             const char* dest);
 
 NODE_DEPRECATED("Use UVException(isolate, ...)",
                 inline v8::Local<v8::Value> ErrnoException(

diff --git a/src/node_file.cc b/src/node_file.cc
index 287d9a56ba116e4d9f5edf5a44fe73abab073e55..b05e9cd55985ba1f83e9b5ff7aec2f80a3786917 100644 (file)
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -109,23 +109,14 @@ static void After(uv_fs_t *req) {
   Local<Value> argv[2];
 
   if (req->result < 0) {
-    // If the request doesn't have a path parameter set.
-    if (req->path == nullptr) {
-      argv[0] = UVException(req->result, nullptr, req_wrap->syscall());
-    } else if ((req->result == UV_EEXIST ||
-                req->result == UV_ENOTEMPTY ||
-                req->result == UV_EPERM) &&
-               req_wrap->dest_len() > 0) {
-      argv[0] = UVException(req->result,
-                            nullptr,
-                            req_wrap->syscall(),
-                            req_wrap->dest());
-    } else {
-      argv[0] = UVException(req->result,
-                            nullptr,
-                            req_wrap->syscall(),
-                            static_cast<const char*>(req->path));
-    }
+    // An error happened.
+    const char* dest = req_wrap->dest_len() > 0 ? req_wrap->dest() : nullptr;
+    argv[0] = UVException(env->isolate(),
+                          req->result,
+                          req_wrap->syscall(),
+                          nullptr,
+                          req->path,
+                          dest);
   } else {
     // error value is empty or null for non-error.
     argv[0] = Null(env->isolate());
@@ -270,14 +261,7 @@ struct fs_req_wrap {
                          __VA_ARGS__,                                         \
                          nullptr);                                            \
   if (err < 0) {                                                              \
-    if (dest != nullptr &&                                                    \
-        (err == UV_EEXIST ||                                                  \
-         err == UV_ENOTEMPTY ||                                               \
-         err == UV_EPERM)) {                                                  \
-      return env->ThrowUVException(err, #func, "", dest);                     \
-    } else {                                                                  \
-      return env->ThrowUVException(err, #func, "", path);                     \
-    }                                                                         \
+    return env->ThrowUVException(err, #func, nullptr, path, dest);            \
   }                                                                           \
 
 #define SYNC_CALL(func, path, ...)                                            \

diff --git a/src/node_internals.h b/src/node_internals.h
index cb8a4a802d37491e02015968a2437f224a5b049e..92da0767befcb8b12bad58fee7ee86f23e3a091d 100644 (file)
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -160,7 +160,8 @@ void ThrowUVException(v8::Isolate* isolate,
                       int errorno,
                       const char* syscall = nullptr,
                       const char* message = nullptr,
-                      const char* path = nullptr);
+                      const char* path = nullptr,
+                      const char* dest = nullptr);
 
 NODE_DEPRECATED("Use ThrowError(isolate)",
                 inline void ThrowError(const char* errmsg) {

diff --git a/test/parallel/test-domain.js b/test/parallel/test-domain.js
index a3af1ff15a4cdffd6b2eea3473246ba651160e03..a3f7b76848a581b9d4c091ea7111441c460c1568 100644 (file)
--- a/test/parallel/test-domain.js
+++ b/test/parallel/test-domain.js
@@ -48,7 +48,7 @@ d.on('error', function(er) {
       assert.equal(er.domainThrown, true);
       break;
 
-    case "ENOENT, open 'this file does not exist'":
+    case "ENOENT: no such file or directory, open 'this file does not exist'":
       assert.equal(er.domain, d);
       assert.equal(er.domainThrown, false);
       assert.equal(typeof er.domainBound, 'function');
@@ -58,7 +58,7 @@ d.on('error', function(er) {
       assert.equal(typeof er.errno, 'number');
       break;
 
-    case "ENOENT, open 'stream for nonexistent file'":
+    case "ENOENT: no such file or directory, open 'stream for nonexistent file'":
       assert.equal(typeof er.errno, 'number');
       assert.equal(er.code, 'ENOENT');
       assert.equal(er_path, 'stream for nonexistent file');

diff --git a/test/parallel/test-fs-error-messages.js b/test/parallel/test-fs-error-messages.js
index 4913ca255de7a888fd1282a99e7e2b3e930bfe3a..e174e1c023ff211e71d06625baa0a227f7eac3ff 100644 (file)
--- a/test/parallel/test-fs-error-messages.js
+++ b/test/parallel/test-fs-error-messages.js
@@ -29,10 +29,12 @@ fs.link(fn, 'foo', function(err) {
 });
 
 fs.link(existingFile, existingFile2, function(err) {
+  assert.ok(0 <= err.message.indexOf(existingFile));
   assert.ok(0 <= err.message.indexOf(existingFile2));
 });
 
 fs.symlink(existingFile, existingFile2, function(err) {
+  assert.ok(0 <= err.message.indexOf(existingFile));
   assert.ok(0 <= err.message.indexOf(existingFile2));
 });
 
@@ -45,6 +47,7 @@ fs.rename(fn, 'foo', function(err) {
 });
 
 fs.rename(existingDir, existingDir2, function(err) {
+  assert.ok(0 <= err.message.indexOf(existingDir));
   assert.ok(0 <= err.message.indexOf(existingDir2));
 });
 
@@ -130,6 +133,7 @@ try {
   fs.linkSync(existingFile, existingFile2);
 } catch (err) {
   errors.push('link');
+  assert.ok(0 <= err.message.indexOf(existingFile));
   assert.ok(0 <= err.message.indexOf(existingFile2));
 }
 
@@ -138,6 +142,7 @@ try {
   fs.symlinkSync(existingFile, existingFile2);
 } catch (err) {
   errors.push('symlink');
+  assert.ok(0 <= err.message.indexOf(existingFile));
   assert.ok(0 <= err.message.indexOf(existingFile2));
 }
 
@@ -186,6 +191,7 @@ try {
   fs.renameSync(existingDir, existingDir2);
 } catch (err) {
   errors.push('rename');
+  assert.ok(0 <= err.message.indexOf(existingDir));
   assert.ok(0 <= err.message.indexOf(existingDir2));
 }