Skip to content

Commit

Permalink
fs: improve error messages
Browse files Browse the repository at this point in the history 
* 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: nodejs#675
Fixes: nodejs#207
Closes: nodejs#293
Reviewed-by: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
@piscisaureus
piscisaureus committed Jan 31, 2015
1 parent 0767c2f commit bc2c85c
Show file tree
Hide file tree
Showing 8 changed files with 92 additions and 79 deletions.
5 changes: 3 additions & 2 deletions src/env-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
4 changes: 3 additions & 1 deletion src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down Expand Up @@ -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);
Expand Down
109 changes: 61 additions & 48 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}


Expand Down Expand Up @@ -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;
}
Expand Down
6 changes: 6 additions & 0 deletions src/node.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
34 changes: 9 additions & 25 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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, ...) \
Expand Down
3 changes: 2 additions & 1 deletion src/node_internals.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-domain.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down
6 changes: 6 additions & 0 deletions test/parallel/test-fs-error-messages.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
});

Expand All @@ -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));
});

Expand Down Expand Up @@ -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));
}

Expand All @@ -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));
}

Expand Down Expand Up @@ -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));
}

Expand Down

0 comments on commit bc2c85c

Please sign in to comment.