fs: return stats to JS in sync methods

- Reduce reference to the global `statValues` by returning
  the changed stats array from the synchronous methods. Having
  a local returned value also makes the future integration
  of BigInt easier.
- Also returns the filled array from node::FillGlobalStatsArray
  and node::FillStatsArray in the C++ side.

PR-URL: https://github.com/nodejs/node/pull/20167
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Gus Caplan <me@gus.host>

lib/fs.js

@@ -57,7 +57,6 @@ const {
   preprocessSymlinkDestination,
   Stats,
   getStatsFromBinding,
-  getStatsFromGlobalBinding,
   stringToFlags,
   stringToSymlinkType,
   toUnixTimestamp,
@@ -158,10 +157,10 @@ function isFd(path) {
 
 fs.Stats = Stats;
 
-function isFileType(fileType) {
+function isFileType(stats, fileType) {
   // Use stats array directly to avoid creating an fs.Stats instance just for
   // our internal use.
-  return (statValues[1/* mode */] & S_IFMT) === fileType;
+  return (stats[1/* mode */] & S_IFMT) === fileType;
 }
 
 // Don't allow mode to accidentally be overwritten.
@@ -330,15 +329,15 @@ function readFileAfterOpen(err, fd) {
   binding.fstat(fd, req);
 }
 
-function readFileAfterStat(err) {
+function readFileAfterStat(err, stats) {
   var context = this.context;
 
   if (err)
     return context.close(err);
 
   var size;
-  if (isFileType(S_IFREG))
+  if (isFileType(stats, S_IFREG))
-    size = context.size = statValues[8];
+    size = context.size = stats[8];
   else
     size = context.size = 0;
 
@@ -411,11 +410,12 @@ function readFileAfterClose(err) {
 
 function tryStatSync(fd, isUserFd) {
   const ctx = {};
-  binding.fstat(fd, undefined, ctx);
+  const stats = binding.fstat(fd, undefined, ctx);
   if (ctx.errno !== undefined && !isUserFd) {
     fs.closeSync(fd);
     throw errors.uvException(ctx);
   }
+  return stats;
 }
 
 function tryCreateBuffer(size, fd, isUserFd) {
@@ -450,10 +450,10 @@ fs.readFileSync = function(path, options) {
   var isUserFd = isFd(path); // file descriptor ownership
   var fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666);
 
-  tryStatSync(fd, isUserFd);
+  const stats = tryStatSync(fd, isUserFd);
   var size;
-  if (isFileType(S_IFREG))
+  if (isFileType(stats, S_IFREG))
-    size = statValues[8];
+    size = stats[8];
   else
     size = 0;
   var pos = 0;
@@ -890,27 +890,29 @@ fs.stat = function(path, callback) {
 fs.fstatSync = function(fd) {
   validateUint32(fd, 'fd');
   const ctx = { fd };
-  binding.fstat(fd, undefined, ctx);
+  const stats = binding.fstat(fd, undefined, ctx);
   handleErrorFromBinding(ctx);
-  return getStatsFromGlobalBinding();
+  return getStatsFromBinding(stats);
 };
 
 fs.lstatSync = function(path) {
   path = getPathFromURL(path);
   validatePath(path);
   const ctx = { path };
-  binding.lstat(pathModule.toNamespacedPath(path), undefined, ctx);
+  const stats = binding.lstat(pathModule.toNamespacedPath(path),
+                              undefined, ctx);
   handleErrorFromBinding(ctx);
-  return getStatsFromGlobalBinding();
+  return getStatsFromBinding(stats);
 };
 
 fs.statSync = function(path) {
   path = getPathFromURL(path);
   validatePath(path);
   const ctx = { path };
-  binding.stat(pathModule.toNamespacedPath(path), undefined, ctx);
+  const stats = binding.stat(pathModule.toNamespacedPath(path),
+                             undefined, ctx);
   handleErrorFromBinding(ctx);
-  return getStatsFromGlobalBinding();
+  return getStatsFromBinding(stats);
 };
 
 fs.readlink = function(path, options, callback) {
@@ -1439,7 +1441,7 @@ function StatWatcher() {
   this._handle.onchange = function(newStatus, stats) {
     if (oldStatus === -1 &&
         newStatus === -1 &&
-        statValues[2/* new nlink */] === statValues[16/* old nlink */]) return;
+        stats[2/* new nlink */] === stats[16/* old nlink */]) return;
 
     oldStatus = newStatus;
     self.emit('change', getStatsFromBinding(stats),
@@ -1666,7 +1668,8 @@ fs.realpathSync = function realpathSync(p, options) {
 
     // continue if not a symlink, break if a pipe/socket
     if (knownHard[base] || (cache && cache.get(base) === base)) {
-      if (isFileType(S_IFIFO) || isFileType(S_IFSOCK)) {
+      if (isFileType(statValues, S_IFIFO) ||
+          isFileType(statValues, S_IFSOCK)) {
         break;
       }
       continue;
@@ -1682,10 +1685,10 @@ fs.realpathSync = function realpathSync(p, options) {
 
       var baseLong = pathModule.toNamespacedPath(base);
       const ctx = { path: base };
-      binding.lstat(baseLong, undefined, ctx);
+      const stats = binding.lstat(baseLong, undefined, ctx);
       handleErrorFromBinding(ctx);
 
-      if (!isFileType(S_IFLNK)) {
+      if (!isFileType(stats, S_IFLNK)) {
         knownHard[base] = true;
         if (cache) cache.set(base, base);
         continue;
@@ -1696,8 +1699,8 @@ fs.realpathSync = function realpathSync(p, options) {
       var linkTarget = null;
       var id;
       if (!isWindows) {
-        var dev = statValues[0].toString(32);
+        var dev = stats[0].toString(32);
-        var ino = statValues[7].toString(32);
+        var ino = stats[7].toString(32);
         id = `${dev}:${ino}`;
         if (seenLinks[id]) {
           linkTarget = seenLinks[id];
@@ -1778,7 +1781,7 @@ fs.realpath = function realpath(p, options, callback) {
 
   // On windows, check that the root exists. On unix there is no need.
   if (isWindows && !knownHard[base]) {
-    fs.lstat(base, function(err) {
+    fs.lstat(base, function(err, stats) {
       if (err) return callback(err);
       knownHard[base] = true;
       LOOP();
@@ -1811,7 +1814,8 @@ fs.realpath = function realpath(p, options, callback) {
 
     // continue if not a symlink, break if a pipe/socket
     if (knownHard[base]) {
-      if (isFileType(S_IFIFO) || isFileType(S_IFSOCK)) {
+      if (isFileType(statValues, S_IFIFO) ||
+          isFileType(statValues, S_IFSOCK)) {
         return callback(null, encodeRealpathResult(p, options));
       }
       return process.nextTick(LOOP);
@@ -1820,14 +1824,11 @@ fs.realpath = function realpath(p, options, callback) {
     return fs.lstat(base, gotStat);
   }
 
-  function gotStat(err) {
+  function gotStat(err, stats) {
     if (err) return callback(err);
 
-    // Use stats array directly to avoid creating an fs.Stats instance just for
-    // our internal use.
-
     // if not a symlink, skip to the next path part
-    if (!isFileType(S_IFLNK)) {
+    if (!stats.isSymbolicLink()) {
       knownHard[base] = true;
       return process.nextTick(LOOP);
     }
@@ -1837,8 +1838,8 @@ fs.realpath = function realpath(p, options, callback) {
     // dev/ino always return 0 on windows, so skip the check.
     let id;
     if (!isWindows) {
-      var dev = statValues[0].toString(32);
+      var dev = stats.dev.toString(32);
-      var ino = statValues[7].toString(32);
+      var ino = stats.ino.toString(32);
       id = `${dev}:${ino}`;
       if (seenLinks[id]) {
         return gotTarget(null, seenLinks[id], base);

lib/internal/fs.js

@@ -35,7 +35,6 @@ const {
   UV_FS_SYMLINK_DIR,
   UV_FS_SYMLINK_JUNCTION
 } = process.binding('constants').fs;
-const { statValues } = process.binding('fs');
 
 const isWindows = process.platform === 'win32';
 
@@ -217,10 +216,6 @@ function getStatsFromBinding(stats, offset = 0) {
                    stats[12 + offset], stats[13 + offset]);
 }
 
-function getStatsFromGlobalBinding(offset = 0) {
-  return getStatsFromBinding(statValues, offset);
-}
-
 function stringToFlags(flags) {
   if (typeof flags === 'number') {
     return flags;
@@ -442,7 +437,6 @@ module.exports = {
   preprocessSymlinkDestination,
   realpathCacheKey: Symbol('realpathCacheKey'),
   getStatsFromBinding,
-  getStatsFromGlobalBinding,
   stringToFlags,
   stringToSymlinkType,
   Stats,

src/node_file.cc

@@ -410,8 +410,7 @@ void FSReqWrap::Reject(Local<Value> reject) {
 }
 
 void FSReqWrap::ResolveStat(const uv_stat_t* stat) {
-  node::FillGlobalStatsArray(env(), stat);
+  Resolve(node::FillGlobalStatsArray(env(), stat));
-  Resolve(env()->fs_stats_field_array()->GetJSArray());
 }
 
 void FSReqWrap::Resolve(Local<Value> value) {
@@ -832,10 +831,13 @@ static void Stat(const FunctionCallbackInfo<Value>& args) {
     FS_SYNC_TRACE_BEGIN(stat);
     int err = SyncCall(env, args[2], &req_wrap_sync, "stat", uv_fs_stat, *path);
     FS_SYNC_TRACE_END(stat);
-    if (err == 0) {
+    if (err != 0) {
-      node::FillGlobalStatsArray(env,
+      return;  // error info is in ctx
-          static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
     }
+
+    Local<Value> arr = node::FillGlobalStatsArray(env,
+        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
+    args.GetReturnValue().Set(arr);
   }
 }
 
@@ -859,10 +861,13 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
     int err = SyncCall(env, args[2], &req_wrap_sync, "lstat", uv_fs_lstat,
                        *path);
     FS_SYNC_TRACE_END(lstat);
-    if (err == 0) {
+    if (err != 0) {
-      node::FillGlobalStatsArray(env,
+      return;  // error info is in ctx
-          static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
     }
+
+    Local<Value> arr = node::FillGlobalStatsArray(env,
+        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
+    args.GetReturnValue().Set(arr);
   }
 }
 
@@ -885,10 +890,13 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
     FS_SYNC_TRACE_BEGIN(fstat);
     int err = SyncCall(env, args[2], &req_wrap_sync, "fstat", uv_fs_fstat, fd);
     FS_SYNC_TRACE_END(fstat);
-    if (err == 0) {
+    if (err != 0) {
-      node::FillGlobalStatsArray(env,
+      return;  // error info is in ctx
-          static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
     }
+
+    Local<Value> arr = node::FillGlobalStatsArray(env,
+        static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
+    args.GetReturnValue().Set(arr);
   }
 }
 

src/node_internals.h

@@ -313,7 +313,7 @@ v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
                                               const char* deprecation_code);
 
 template <typename NativeT, typename V8T>
-void FillStatsArray(AliasedBuffer<NativeT, V8T>* fields_ptr,
+v8::Local<v8::Value> FillStatsArray(AliasedBuffer<NativeT, V8T>* fields_ptr,
                     const uv_stat_t* s, int offset = 0) {
   AliasedBuffer<NativeT, V8T>& fields = *fields_ptr;
   fields[offset + 0] = s->st_dev;
@@ -347,12 +347,14 @@ void FillStatsArray(AliasedBuffer<NativeT, V8T>* fields_ptr,
   X(12, ctim)
   X(13, birthtim)
 #undef X
+
+  return fields_ptr->GetJSArray();
 }
 
-inline void FillGlobalStatsArray(Environment* env,
+inline v8::Local<v8::Value> FillGlobalStatsArray(Environment* env,
-                                 const uv_stat_t* s,
+                                                 const uv_stat_t* s,
-                                 int offset = 0) {
+                                                 int offset = 0) {
-  node::FillStatsArray(env->fs_stats_field_array(), s, offset);
+  return node::FillStatsArray(env->fs_stats_field_array(), s, offset);
 }
 
 void SetupProcessObject(Environment* env,

src/node_stat_watcher.cc

@@ -108,12 +108,12 @@ void StatWatcher::Callback(uv_fs_poll_t* handle,
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(env->context());
 
-  node::FillGlobalStatsArray(env, curr);
+  Local<Value> arr = node::FillGlobalStatsArray(env, curr);
   node::FillGlobalStatsArray(env, prev, env->kFsStatsFieldsLength);
 
   Local<Value> argv[2] {
     Integer::New(env->isolate(), status),
-    env->fs_stats_field_array()->GetJSArray()
+    arr
   };
   wrap->MakeCallback(env->onchange_string(), arraysize(argv), argv);
 }