Skip to content

Commit 92bfa64

Browse files
piscisaureuspiscisaureus
committed
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. API impact: * The public node::UVException() function now takes 6 arguments.
1 parent 3d4e96f commit 92bfa64

File tree

8 files changed

+78
-79
lines changed

8 files changed

+78
-79
lines changed

‎src/env-inl.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -377,9 +377,10 @@ inline void Environment::ThrowErrnoException(int errorno,
377377
inline void Environment::ThrowUVException(int errorno,
378378
const char* syscall,
379379
const char* message,
380-
const char* path) {
380+
const char* path,
381+
const char* dest) {
381382
isolate()->ThrowException(
382-
UVException(isolate(), errorno, syscall, message, path));
383+
UVException(isolate(), errorno, syscall, message, path, dest));
383384
}
384385

385386
inline v8::Local<v8::FunctionTemplate>

‎src/env.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ namespace node {
6161
V(cwd_string, "cwd") \
6262
V(debug_port_string, "debugPort") \
6363
V(debug_string, "debug") \
64+
V(dest_string, "dest") \
6465
V(detached_string, "detached") \
6566
V(dev_string, "dev") \
6667
V(disposed_string, "_disposed") \
@@ -407,7 +408,8 @@ class Environment {
407408
inline void ThrowUVException(int errorno,
408409
const char* syscall = nullptr,
409410
const char* message = nullptr,
410-
const char* path = nullptr);
411+
const char* path = nullptr,
412+
const char* dest = nullptr);
411413

412414
// Convenience methods for contextify
413415
inline static void ThrowError(v8::Isolate* isolate, const char* errmsg);

‎src/node.cc‎

Lines changed: 51 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -698,11 +698,10 @@ void ThrowUVException(v8::Isolate* isolate,
698698
int errorno,
699699
const char* syscall,
700700
const char* message,
701-
const char* path) {
702-
Environment::GetCurrent(isolate)->ThrowErrnoException(errorno,
703-
syscall,
704-
message,
705-
path);
701+
const char* path,
702+
const char* dest) {
703+
Environment::GetCurrent(isolate)
704+
->ThrowUVException(errorno, syscall, message, path, dest);
706705
}
707706

708707

@@ -752,64 +751,69 @@ Local<Value> ErrnoException(Isolate* isolate,
752751
}
753752

754753

754+
static Local<String> StringFromPath(Isolate* isolate, const char* path) {
755+
#ifdef _WIN32
756+
if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) {
757+
return String::Concat(FIXED_ONE_BYTE_STRING(isolate, "\\\\"),
758+
String::NewFromUtf8(isolate, path + 8));
759+
} else if (strncmp(path, "\\\\?\\", 4) == 0) {
760+
return String::NewFromUtf8(isolate, path + 4);
761+
}
762+
#endif
763+
764+
return String::NewFromUtf8(isolate, path);
765+
}
766+
767+
755768
// hack alert! copy of ErrnoException, tuned for uv errors
756769
Local<Value> UVException(Isolate* isolate,
757770
int errorno,
758-
const char *syscall,
759-
const char *msg,
760-
const char *path) {
771+
const char* syscall,
772+
const char* msg,
773+
const char* path,
774+
const char* dest) {
761775
Environment* env = Environment::GetCurrent(isolate);
762776

763777
if (!msg || !msg[0])
764778
msg = uv_strerror(errorno);
765779

766-
Local<String> estring = OneByteString(env->isolate(), uv_err_name(errorno));
767-
Local<String> message = OneByteString(env->isolate(), msg);
768-
Local<String> cons1 =
769-
String::Concat(estring, FIXED_ONE_BYTE_STRING(env->isolate(), ", "));
770-
Local<String> cons2 = String::Concat(cons1, message);
771-
772-
Local<Value> e;
780+
Local<String> js_code = OneByteString(isolate, uv_err_name(errorno));
781+
Local<String> js_syscall = OneByteString(isolate, syscall);
782+
Local<String> js_path;
783+
Local<String> js_dest;
773784

774-
Local<String> path_str;
785+
Local<String> js_msg = js_code;
786+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, ": "));
787+
js_msg = String::Concat(js_msg, OneByteString(isolate, msg));
788+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, ", "));
789+
js_msg = String::Concat(js_msg, js_syscall);
775790

776-
if (path) {
777-
#ifdef _WIN32
778-
if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) {
779-
path_str = String::Concat(FIXED_ONE_BYTE_STRING(env->isolate(), "\\\\"),
780-
String::NewFromUtf8(env->isolate(), path + 8));
781-
} else if (strncmp(path, "\\\\?\\", 4) == 0) {
782-
path_str = String::NewFromUtf8(env->isolate(), path + 4);
783-
} else {
784-
path_str = String::NewFromUtf8(env->isolate(), path);
785-
}
786-
#else
787-
path_str = String::NewFromUtf8(env->isolate(), path);
788-
#endif
791+
if (path != nullptr) {
792+
js_path = StringFromPath(isolate, path);
789793

790-
Local<String> cons3 =
791-
String::Concat(cons2, FIXED_ONE_BYTE_STRING(env->isolate(), " '"));
792-
Local<String> cons4 =
793-
String::Concat(cons3, path_str);
794-
Local<String> cons5 =
795-
String::Concat(cons4, FIXED_ONE_BYTE_STRING(env->isolate(), "'"));
796-
e = Exception::Error(cons5);
797-
} else {
798-
e = Exception::Error(cons2);
794+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, " '"));
795+
js_msg = String::Concat(js_msg, js_path);
796+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, "'"));
799797
}
800798

801-
Local<Object> obj = e->ToObject(env->isolate());
802-
// TODO(piscisaureus) errno should probably go
803-
obj->Set(env->errno_string(), Integer::New(env->isolate(), errorno));
804-
obj->Set(env->code_string(), estring);
799+
if (dest != nullptr) {
800+
js_dest = StringFromPath(isolate, dest);
805801

806-
if (path != nullptr) {
807-
obj->Set(env->path_string(), path_str);
802+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, " -> '"));
803+
js_msg = String::Concat(js_msg, js_dest);
804+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, "'"));
808805
}
809806

810-
if (syscall != nullptr) {
811-
obj->Set(env->syscall_string(), OneByteString(env->isolate(), syscall));
812-
}
807+
Local<Object> e = Exception::Error(js_msg)->ToObject(isolate);
808+
809+
// TODO(piscisaureus) errorno should probably go
810+
e->Set(env->errno_string(), Integer::New(isolate, errorno));
811+
e->Set(env->code_string(), js_code);
812+
e->Set(env->syscall_string(), js_syscall);
813+
if (!js_path.IsEmpty())
814+
e->Set(env->path_string(), js_path);
815+
if (!js_dest.IsEmpty())
816+
e->Set(env->dest_string(), js_dest);
813817

814818
return e;
815819
}

‎src/node.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,8 @@ NODE_EXTERN v8::Local<v8::Value> UVException(v8::Isolate* isolate,
5858
int errorno,
5959
const char* syscall = NULL,
6060
const char* message = NULL,
61-
const char* path = NULL);
61+
const char* path = NULL,
62+
const char* dest = NULL);
6263

6364
NODE_DEPRECATED("Use UVException(isolate, ...)",
6465
inline v8::Local<v8::Value> ErrnoException(

‎src/node_file.cc‎

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -109,23 +109,14 @@ static void After(uv_fs_t *req) {
109109
Local<Value> argv[2];
110110

111111
if (req->result < 0) {
112-
// If the request doesn't have a path parameter set.
113-
if (req->path == nullptr) {
114-
argv[0] = UVException(req->result, nullptr, req_wrap->syscall());
115-
} else if ((req->result == UV_EEXIST ||
116-
req->result == UV_ENOTEMPTY ||
117-
req->result == UV_EPERM) &&
118-
req_wrap->dest_len() > 0) {
119-
argv[0] = UVException(req->result,
120-
nullptr,
121-
req_wrap->syscall(),
122-
req_wrap->dest());
123-
} else {
124-
argv[0] = UVException(req->result,
125-
nullptr,
126-
req_wrap->syscall(),
127-
static_cast<const char*>(req->path));
128-
}
112+
// An error happened.
113+
const char* dest = req_wrap->dest_len() > 0 ? req_wrap->dest() : nullptr;
114+
argv[0] = UVException(env->isolate(),
115+
req->result,
116+
req_wrap->syscall(),
117+
nullptr,
118+
req->path,
119+
dest);
129120
} else {
130121
// error value is empty or null for non-error.
131122
argv[0] = Null(env->isolate());
@@ -270,14 +261,7 @@ struct fs_req_wrap {
270261
__VA_ARGS__, \
271262
nullptr); \
272263
if (err < 0) { \
273-
if (dest != nullptr && \
274-
(err == UV_EEXIST || \
275-
err == UV_ENOTEMPTY || \
276-
err == UV_EPERM)) { \
277-
return env->ThrowUVException(err, #func, "", dest); \
278-
} else { \
279-
return env->ThrowUVException(err, #func, "", path); \
280-
} \
264+
return env->ThrowUVException(err, #func, nullptr, path, dest); \
281265
} \
282266

283267
#define SYNC_CALL(func, path, ...) \

‎src/node_internals.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,8 @@ void ThrowUVException(v8::Isolate* isolate,
160160
int errorno,
161161
const char* syscall = nullptr,
162162
const char* message = nullptr,
163-
const char* path = nullptr);
163+
const char* path = nullptr,
164+
const char* dest = nullptr);
164165

165166
NODE_DEPRECATED("Use ThrowError(isolate)",
166167
inline void ThrowError(const char* errmsg) {

‎test/parallel/test-domain.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ d.on('error', function(er) {
4848
assert.equal(er.domainThrown, true);
4949
break;
5050

51-
case "ENOENT, open 'this file does not exist'":
51+
case "ENOENT: no such file or directory, open 'this file does not exist'":
5252
assert.equal(er.domain, d);
5353
assert.equal(er.domainThrown, false);
5454
assert.equal(typeof er.domainBound, 'function');
@@ -58,7 +58,7 @@ d.on('error', function(er) {
5858
assert.equal(typeof er.errno, 'number');
5959
break;
6060

61-
case "ENOENT, open 'stream for nonexistent file'":
61+
case "ENOENT: no such file or directory, open 'stream for nonexistent file'":
6262
assert.equal(typeof er.errno, 'number');
6363
assert.equal(er.code, 'ENOENT');
6464
assert.equal(er_path, 'stream for nonexistent file');

‎test/parallel/test-fs-error-messages.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,12 @@ fs.link(fn, 'foo', function(err) {
2929
});
3030

3131
fs.link(existingFile, existingFile2, function(err) {
32+
assert.ok(0 <= err.message.indexOf(existingFile));
3233
assert.ok(0 <= err.message.indexOf(existingFile2));
3334
});
3435

3536
fs.symlink(existingFile, existingFile2, function(err) {
37+
assert.ok(0 <= err.message.indexOf(existingFile));
3638
assert.ok(0 <= err.message.indexOf(existingFile2));
3739
});
3840

@@ -45,6 +47,7 @@ fs.rename(fn, 'foo', function(err) {
4547
});
4648

4749
fs.rename(existingDir, existingDir2, function(err) {
50+
assert.ok(0 <= err.message.indexOf(existingDir));
4851
assert.ok(0 <= err.message.indexOf(existingDir2));
4952
});
5053

@@ -130,6 +133,7 @@ try {
130133
fs.linkSync(existingFile, existingFile2);
131134
} catch (err) {
132135
errors.push('link');
136+
assert.ok(0 <= err.message.indexOf(existingFile));
133137
assert.ok(0 <= err.message.indexOf(existingFile2));
134138
}
135139

@@ -138,6 +142,7 @@ try {
138142
fs.symlinkSync(existingFile, existingFile2);
139143
} catch (err) {
140144
errors.push('symlink');
145+
assert.ok(0 <= err.message.indexOf(existingFile));
141146
assert.ok(0 <= err.message.indexOf(existingFile2));
142147
}
143148

@@ -186,6 +191,7 @@ try {
186191
fs.renameSync(existingDir, existingDir2);
187192
} catch (err) {
188193
errors.push('rename');
194+
assert.ok(0 <= err.message.indexOf(existingDir));
189195
assert.ok(0 <= err.message.indexOf(existingDir2));
190196
}
191197

0 commit comments

Comments
 (0)