fs: simplify the error context collection in C++, migrate fs.close errors

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) {

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,30 +414,38 @@ 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);
   }
 }
 
 
 void Close(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
 
+  int length = args.Length();
+  CHECK_GE(length, 2);
   CHECK(args[0]->IsInt32());
 
-  int fd = args[0]->Int32Value();
+  int fd = static_cast<int>(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<Object> 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);
   }
 }
 

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<Object> target,
                   Local<Value> unused,
                   Local<Context> 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<Map> err_map = Map::New(isolate);
+
+#define V(name, msg) do {                                                     \
+  Local<Array> 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

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

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

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

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(() => {