fs: simplify the error context collection in C++

- Simplify the SyncCall template function, only collect error
  number and syscall in the C++ layer and collect the rest of context
  in JS for flexibility.
- Remove the stringFromPath JS helper now that the unprefixed path is
  directly put into the context before the binding is invoked with the
  prefixed path.
- Validate more properties in fs.access tests.

PR-URL: #17338
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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);
   }
 };

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;
 }

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<Object> 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 <typename Func, typename... Args>
-inline void SyncDestCall(Environment* env, Local<Value> ctx,
-    const char* path, const char* dest, const char* syscall,
-    Func fn, Args... args) {
+inline void SyncCall(Environment* env, Local<Value> 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> context = env->context();
     Local<Object> 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 <typename Func, typename... Args>
-inline void SyncCall(Environment* env, Local<Value> 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<Value>& args) {
   BufferValue path(env->isolate(), args[0]);
   int mode = static_cast<int>(args[1]->Int32Value(context).FromJust());
 
-  if (args[2]->IsObject()) {
+  if (args[2]->IsObject()) {  // access(path, mode, req)
     Local<Object> 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);
   }
 }
 

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;
   }
 );