Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: improve error messages #675

Merged
merged 1 commit into from
Jan 31, 2015
Merged

fs: improve error messages #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/env-inl.h
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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