diff --git a/lib/fs.js b/lib/fs.js index 7741e5838bd216..c6c31983558a1d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -329,10 +329,10 @@ fs.accessSync = function(path, mode) { else mode = mode | 0; - const ctx = {}; + const ctx = { path }; binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx); - if (ctx.code !== undefined) { + if (ctx.errno !== undefined) { throw new errors.uvException(ctx); } }; @@ -651,7 +651,11 @@ fs.closeSync = function(fd) { if (!isUint32(fd)) throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'integer'); - return binding.close(fd); + const ctx = {}; + binding.close(fd, undefined, ctx); + if (ctx.errno !== undefined) { + throw new errors.uvException(ctx); + } }; function modeNum(m, def) { diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 9e3d1a9355e4f0..db501a1d3cfec8 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -14,6 +14,7 @@ const kCode = Symbol('code'); const kInfo = Symbol('info'); const messages = new Map(); +const { errmap } = process.binding('uv'); const { kMaxLength } = process.binding('buffer'); const { defineProperty } = Object; @@ -194,43 +195,35 @@ function E(sym, val) { messages.set(sym, typeof val === 'function' ? val : String(val)); } -// JS counterpart of StringFromPath, although here path is a buffer. -function stringFromPath(path) { - const str = path.toString(); - if (process.platform !== 'win32') { - return str; - } - - if (str.startsWith('\\\\?\\UNC\\')) { - return '\\\\' + str.slice(8); - } else if (str.startsWith('\\\\?\\')) { - return str.slice(4); - } - return str; -} - // This creates an error compatible with errors produced in UVException // using the context collected in CollectUVExceptionInfo // The goal is to migrate them to ERR_* errors later when // compatibility is not a concern function uvException(ctx) { const err = new Error(); - err.errno = ctx.errno; - err.code = ctx.code; - err.syscall = ctx.syscall; - let message = `${ctx.code}: ${ctx.message}, ${ctx.syscall}`; + for (const prop of Object.keys(ctx)) { + if (prop === 'message' || prop === 'path' || prop === 'dest') { + continue; + } + err[prop] = ctx[prop]; + } + + const [ code, uvmsg ] = errmap.get(ctx.errno); + err.code = code; + let message = `${code}: ${uvmsg}, ${ctx.syscall}`; if (ctx.path) { - const path = stringFromPath(ctx.path); + const path = ctx.path.toString(); message += ` '${path}'`; err.path = path; } if (ctx.dest) { - const dest = stringFromPath(ctx.dest); + const dest = ctx.dest.toString(); message += ` -> '${dest}'`; err.dest = dest; } err.message = message; + Error.captureStackTrace(err, uvException); return err; } diff --git a/src/node_file.cc b/src/node_file.cc index 272e0906447357..3f680c14d64702 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -88,6 +88,7 @@ using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; using v8::Integer; +using v8::Isolate; using v8::Local; using v8::MaybeLocal; using v8::Number; @@ -157,6 +158,15 @@ FSReqAfterScope::~FSReqAfterScope() { wrap_->Dispose(); } +// TODO(joyeecheung): create a normal context object, and +// construct the actual errors in the JS land using the context. +// The context should include fds for some fs APIs, currently they are +// missing in the error messages. The path, dest, syscall, fd, .etc +// can be put into the context before the binding is even invoked, +// the only information that has to come from the C++ layer is the +// error number (and possibly the syscall for abstraction), +// which is also why the errors should have been constructed +// in JS for more flexibility. void FSReqAfterScope::Reject(uv_fs_t* req) { wrap_->Reject(UVException(wrap_->env()->isolate(), req->result, @@ -354,28 +364,28 @@ inline FSReqWrap* AsyncCall(Environment* env, Local req, #define ASYNC_CALL(after, func, req, encoding, ...) \ ASYNC_DEST_CALL(after, func, req, nullptr, encoding, __VA_ARGS__) \ -// Template counterpart of SYNC_DEST_CALL +// Template counterpart of SYNC_CALL, except that it only puts +// the error number and the syscall in the context instead of +// creating an error in the C++ land. template -inline void SyncDestCall(Environment* env, Local ctx, - const char* path, const char* dest, const char* syscall, - Func fn, Args... args) { +inline void SyncCall(Environment* env, Local ctx, + const char* syscall, Func fn, Args... args) { fs_req_wrap req_wrap; env->PrintSyncTrace(); int err = fn(env->event_loop(), &req_wrap.req, args..., nullptr); - if (err) { + if (err < 0) { Local context = env->context(); Local ctx_obj = ctx->ToObject(context).ToLocalChecked(); - env->CollectUVExceptionInfo(ctx_obj, err, syscall, nullptr, path, dest); + Isolate *isolate = env->isolate(); + ctx_obj->Set(context, + env->errno_string(), + Integer::New(isolate, err)).FromJust(); + ctx_obj->Set(context, + env->syscall_string(), + OneByteString(isolate, syscall)).FromJust(); } } -// Template counterpart of SYNC_CALL -template -inline void SyncCall(Environment* env, Local ctx, - const char* path, const char* syscall, Func fn, Args... args) { - return SyncDestCall(env, ctx, path, nullptr, syscall, fn, args...); -} - #define SYNC_DEST_CALL(func, path, dest, ...) \ fs_req_wrap req_wrap; \ env->PrintSyncTrace(); \ @@ -404,30 +414,38 @@ void Access(const FunctionCallbackInfo& args) { BufferValue path(env->isolate(), args[0]); int mode = static_cast(args[1]->Int32Value(context).FromJust()); - if (args[2]->IsObject()) { + if (args[2]->IsObject()) { // access(path, mode, req) Local req_obj = args[2]->ToObject(context).ToLocalChecked(); FSReqWrap* req_wrap = AsyncCall( env, req_obj, UTF8, "access", AfterNoArgs, uv_fs_access, *path, mode); if (req_wrap != nullptr) { args.GetReturnValue().Set(req_wrap->persistent()); } - } else { - SyncCall(env, args[3], *path, "access", uv_fs_access, *path, mode); + } else { // access(path, mode, undefined, ctx) + SyncCall(env, args[3], "access", uv_fs_access, *path, mode); } } void Close(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + Local context = env->context(); + int length = args.Length(); + CHECK_GE(length, 2); CHECK(args[0]->IsInt32()); - int fd = args[0]->Int32Value(); + int fd = static_cast(args[0]->Int32Value(context).FromJust()); - if (args[1]->IsObject()) { - ASYNC_CALL(AfterNoArgs, close, args[1], UTF8, fd) - } else { - SYNC_CALL(close, 0, fd) + if (args[1]->IsObject()) { // close(fd, req) + Local req_obj = args[1]->ToObject(context).ToLocalChecked(); + FSReqWrap* req_wrap = AsyncCall( + env, req_obj, UTF8, "close", AfterNoArgs, uv_fs_close, fd); + if (req_wrap != nullptr) { + args.GetReturnValue().Set(req_wrap->persistent()); + } + } else { // close(fd, undefined, ctx) + SyncCall(env, args[2], "close", uv_fs_close, fd); } } diff --git a/src/uv.cc b/src/uv.cc index f70da1baae5deb..3c115cf05b5a3a 100644 --- a/src/uv.cc +++ b/src/uv.cc @@ -27,10 +27,15 @@ namespace node { namespace { +using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; +using v8::Integer; +using v8::Isolate; using v8::Local; +using v8::Map; using v8::Object; +using v8::String; using v8::Value; @@ -47,14 +52,30 @@ void InitializeUV(Local target, Local unused, Local context) { Environment* env = Environment::GetCurrent(context); - target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "errname"), + Isolate* isolate = env->isolate(); + target->Set(FIXED_ONE_BYTE_STRING(isolate, "errname"), env->NewFunctionTemplate(ErrName)->GetFunction()); #define V(name, _) NODE_DEFINE_CONSTANT(target, UV_##name); UV_ERRNO_MAP(V) #undef V -} + Local err_map = Map::New(isolate); + +#define V(name, msg) do { \ + Local arr = Array::New(isolate, 2); \ + arr->Set(0, OneByteString(isolate, #name)); \ + arr->Set(1, OneByteString(isolate, msg)); \ + err_map->Set(context, \ + Integer::New(isolate, UV_##name), \ + arr).ToLocalChecked(); \ +} while (0); + UV_ERRNO_MAP(V) +#undef V + + target->Set(context, FIXED_ONE_BYTE_STRING(isolate, "errmap"), + err_map).FromJust(); +} } // anonymous namespace } // namespace node diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js index 719e1108fe1c02..9811f9340873b7 100644 --- a/test/parallel/test-fs-access.js +++ b/test/parallel/test-fs-access.js @@ -1,8 +1,14 @@ 'use strict'; + +// This tests that fs.access and fs.accessSync works as expected +// and the errors thrown from these APIs include the desired properties + const common = require('../common'); const assert = require('assert'); const fs = require('fs'); const path = require('path'); +const uv = process.binding('uv'); + const doesNotExist = path.join(common.tmpDir, '__this_should_not_exist'); const readOnlyFile = path.join(common.tmpDir, 'read_only_file'); const readWriteFile = path.join(common.tmpDir, 'read_write_file'); @@ -130,6 +136,24 @@ assert.throws( `ENOENT: no such file or directory, access '${doesNotExist}'` ); assert.strictEqual(err.constructor, Error); + assert.strictEqual(err.syscall, 'access'); + assert.strictEqual(err.errno, uv.UV_ENOENT); + return true; + } +); + +assert.throws( + () => { fs.accessSync(Buffer.from(doesNotExist)); }, + (err) => { + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.path, doesNotExist); + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, access '${doesNotExist}'` + ); + assert.strictEqual(err.constructor, Error); + assert.strictEqual(err.syscall, 'access'); + assert.strictEqual(err.errno, uv.UV_ENOENT); return true; } ); diff --git a/test/parallel/test-fs-close-errors.js b/test/parallel/test-fs-close-errors.js index cadcf2f78c4aa4..4f0d31369a2651 100644 --- a/test/parallel/test-fs-close-errors.js +++ b/test/parallel/test-fs-close-errors.js @@ -1,7 +1,12 @@ 'use strict'; +// This tests that the errors thrown from fs.close and fs.closeSync +// include the desired properties + const common = require('../common'); +const assert = require('assert'); const fs = require('fs'); +const uv = process.binding('uv'); ['', false, null, undefined, {}, []].forEach((i) => { common.expectsError( @@ -21,3 +26,41 @@ const fs = require('fs'); } ); }); + +{ + assert.throws( + () => { + const fd = fs.openSync(__filename, 'r'); + fs.closeSync(fd); + fs.closeSync(fd); + }, + (err) => { + assert.strictEqual(err.code, 'EBADF'); + assert.strictEqual( + err.message, + 'EBADF: bad file descriptor, close' + ); + assert.strictEqual(err.constructor, Error); + assert.strictEqual(err.syscall, 'close'); + assert.strictEqual(err.errno, uv.UV_EBADF); + return true; + } + ); +} + +{ + const fd = fs.openSync(__filename, 'r'); + fs.close(fd, common.mustCall((err) => { + assert.ifError(err); + fs.close(fd, common.mustCall((err) => { + assert.strictEqual(err.code, 'EBADF'); + assert.strictEqual( + err.message, + 'EBADF: bad file descriptor, close' + ); + assert.strictEqual(err.constructor, Error); + assert.strictEqual(err.syscall, 'close'); + assert.strictEqual(err.errno, uv.UV_EBADF); + })); + })); +} diff --git a/test/parallel/test-uv-binding-constant.js b/test/parallel/test-uv-binding-constant.js index cad08575ca92eb..beae7672db12f5 100644 --- a/test/parallel/test-uv-binding-constant.js +++ b/test/parallel/test-uv-binding-constant.js @@ -9,10 +9,10 @@ const uv = process.binding('uv'); const keys = Object.keys(uv); keys.forEach((key) => { - if (key === 'errname') - return; // skip this - const val = uv[key]; - assert.throws(() => uv[key] = 1, - /^TypeError: Cannot assign to read only property/); - assert.strictEqual(uv[key], val); + if (key.startsWith('UV_')) { + const val = uv[key]; + assert.throws(() => uv[key] = 1, + /^TypeError: Cannot assign to read only property/); + assert.strictEqual(uv[key], val); + } }); diff --git a/test/parallel/test-uv-errno.js b/test/parallel/test-uv-errno.js index d5ae7548a040d7..223be9058050d6 100644 --- a/test/parallel/test-uv-errno.js +++ b/test/parallel/test-uv-errno.js @@ -8,7 +8,7 @@ const uv = process.binding('uv'); const keys = Object.keys(uv); keys.forEach((key) => { - if (key === 'errname') + if (!key.startsWith('UV_')) return; assert.doesNotThrow(() => {