Skip to content

Commit bc2c85c

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. * 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: #675 Fixes: #207 Closes: #293 Reviewed-by: Ben Noordhuis <[email protected]>
1 parent 0767c2f commit bc2c85c

File tree

8 files changed

+92
-79
lines changed

8 files changed

+92
-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: 61 additions & 48 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,78 @@ Local<Value> ErrnoException(Isolate* isolate,
752751
}
753752

754753

755-
// hack alert! copy of ErrnoException, tuned for uv errors
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+
768+
Local<Value> UVException(Isolate* isolate,
769+
int errorno,
770+
const char* syscall,
771+
const char* msg,
772+
const char* path) {
773+
return UVException(isolate, errorno, syscall, msg, path, nullptr);
774+
}
775+
776+
756777
Local<Value> UVException(Isolate* isolate,
757778
int errorno,
758-
const char *syscall,
759-
const char *msg,
760-
const char *path) {
779+
const char* syscall,
780+
const char* msg,
781+
const char* path,
782+
const char* dest) {
761783
Environment* env = Environment::GetCurrent(isolate);
762784

763785
if (!msg || !msg[0])
764786
msg = uv_strerror(errorno);
765787

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;
788+
Local<String> js_code = OneByteString(isolate, uv_err_name(errorno));
789+
Local<String> js_syscall = OneByteString(isolate, syscall);
790+
Local<String> js_path;
791+
Local<String> js_dest;
773792

774-
Local<String> path_str;
793+
Local<String> js_msg = js_code;
794+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, ": "));
795+
js_msg = String::Concat(js_msg, OneByteString(isolate, msg));
796+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, ", "));
797+
js_msg = String::Concat(js_msg, js_syscall);
775798

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
799+
if (path != nullptr) {
800+
js_path = StringFromPath(isolate, path);
789801

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);
802+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, " '"));
803+
js_msg = String::Concat(js_msg, js_path);
804+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, "'"));
799805
}
800806

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);
807+
if (dest != nullptr) {
808+
js_dest = StringFromPath(isolate, dest);
805809

806-
if (path != nullptr) {
807-
obj->Set(env->path_string(), path_str);
810+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, " -> '"));
811+
js_msg = String::Concat(js_msg, js_dest);
812+
js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, "'"));
808813
}
809814

810-
if (syscall != nullptr) {
811-
obj->Set(env->syscall_string(), OneByteString(env->isolate(), syscall));
812-
}
815+
Local<Object> e = Exception::Error(js_msg)->ToObject(isolate);
816+
817+
// TODO(piscisaureus) errno should probably go; the user has no way of
818+
// knowing which uv errno value maps to which error.
819+
e->Set(env->errno_string(), Integer::New(isolate, errorno));
820+
e->Set(env->code_string(), js_code);
821+
e->Set(env->syscall_string(), js_syscall);
822+
if (!js_path.IsEmpty())
823+
e->Set(env->path_string(), js_path);
824+
if (!js_dest.IsEmpty())
825+
e->Set(env->dest_string(), js_dest);
813826

814827
return e;
815828
}

‎src/node.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,12 @@ NODE_EXTERN v8::Local<v8::Value> UVException(v8::Isolate* isolate,
5959
const char* syscall = NULL,
6060
const char* message = NULL,
6161
const char* path = NULL);
62+
NODE_EXTERN v8::Local<v8::Value> UVException(v8::Isolate* isolate,
63+
int errorno,
64+
const char* syscall,
65+
const char* message,
66+
const char* path,
67+
const char* dest);
6268

6369
NODE_DEPRECATED("Use UVException(isolate, ...)",
6470
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)