diff --git a/lib/fs.js b/lib/fs.js index 93be37533405da..f357ff2b9d7691 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); } }; 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..3373ad8707e225 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,15 +414,15 @@ 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); } } 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; } );