Skip to content

Commit 65e62e5

Browse files
joyeecheungjasnell
joyeecheung
authored and
jasnell
committed
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: #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>
1 parent b7d1e19 commit 65e62e5

File tree

5 files changed

+60
-55
lines changed

5 files changed

+60
-55
lines changed

‎lib/fs.js‎

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ const {
5757
preprocessSymlinkDestination,
5858
Stats,
5959
getStatsFromBinding,
60-
getStatsFromGlobalBinding,
6160
stringToFlags,
6261
stringToSymlinkType,
6362
toUnixTimestamp,
@@ -158,10 +157,10 @@ function isFd(path) {
158157

159158
fs.Stats = Stats;
160159

161-
function isFileType(fileType) {
160+
function isFileType(stats, fileType) {
162161
// Use stats array directly to avoid creating an fs.Stats instance just for
163162
// our internal use.
164-
return (statValues[1/* mode */] & S_IFMT) === fileType;
163+
return (stats[1/* mode */] & S_IFMT) === fileType;
165164
}
166165

167166
// Don't allow mode to accidentally be overwritten.
@@ -330,15 +329,15 @@ function readFileAfterOpen(err, fd) {
330329
binding.fstat(fd, req);
331330
}
332331

333-
function readFileAfterStat(err) {
332+
function readFileAfterStat(err, stats) {
334333
var context = this.context;
335334

336335
if (err)
337336
return context.close(err);
338337

339338
var size;
340-
if (isFileType(S_IFREG))
341-
size = context.size = statValues[8];
339+
if (isFileType(stats, S_IFREG))
340+
size = context.size = stats[8];
342341
else
343342
size = context.size = 0;
344343

@@ -411,11 +410,12 @@ function readFileAfterClose(err) {
411410

412411
function tryStatSync(fd, isUserFd) {
413412
const ctx = {};
414-
binding.fstat(fd, undefined, ctx);
413+
const stats = binding.fstat(fd, undefined, ctx);
415414
if (ctx.errno !== undefined && !isUserFd) {
416415
fs.closeSync(fd);
417416
throw errors.uvException(ctx);
418417
}
418+
return stats;
419419
}
420420

421421
function tryCreateBuffer(size, fd, isUserFd) {
@@ -450,10 +450,10 @@ fs.readFileSync = function(path, options) {
450450
var isUserFd = isFd(path); // file descriptor ownership
451451
var fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666);
452452

453-
tryStatSync(fd, isUserFd);
453+
const stats = tryStatSync(fd, isUserFd);
454454
var size;
455-
if (isFileType(S_IFREG))
456-
size = statValues[8];
455+
if (isFileType(stats, S_IFREG))
456+
size = stats[8];
457457
else
458458
size = 0;
459459
var pos = 0;
@@ -890,27 +890,29 @@ fs.stat = function(path, callback) {
890890
fs.fstatSync = function(fd) {
891891
validateUint32(fd, 'fd');
892892
const ctx = { fd };
893-
binding.fstat(fd, undefined, ctx);
893+
const stats = binding.fstat(fd, undefined, ctx);
894894
handleErrorFromBinding(ctx);
895-
return getStatsFromGlobalBinding();
895+
return getStatsFromBinding(stats);
896896
};
897897

898898
fs.lstatSync = function(path) {
899899
path = getPathFromURL(path);
900900
validatePath(path);
901901
const ctx = { path };
902-
binding.lstat(pathModule.toNamespacedPath(path), undefined, ctx);
902+
const stats = binding.lstat(pathModule.toNamespacedPath(path),
903+
undefined, ctx);
903904
handleErrorFromBinding(ctx);
904-
return getStatsFromGlobalBinding();
905+
return getStatsFromBinding(stats);
905906
};
906907

907908
fs.statSync = function(path) {
908909
path = getPathFromURL(path);
909910
validatePath(path);
910911
const ctx = { path };
911-
binding.stat(pathModule.toNamespacedPath(path), undefined, ctx);
912+
const stats = binding.stat(pathModule.toNamespacedPath(path),
913+
undefined, ctx);
912914
handleErrorFromBinding(ctx);
913-
return getStatsFromGlobalBinding();
915+
return getStatsFromBinding(stats);
914916
};
915917

916918
fs.readlink = function(path, options, callback) {
@@ -1439,7 +1441,7 @@ function StatWatcher() {
14391441
this._handle.onchange = function(newStatus, stats) {
14401442
if (oldStatus === -1 &&
14411443
newStatus === -1 &&
1442-
statValues[2/* new nlink */] === statValues[16/* old nlink */]) return;
1444+
stats[2/* new nlink */] === stats[16/* old nlink */]) return;
14431445

14441446
oldStatus = newStatus;
14451447
self.emit('change', getStatsFromBinding(stats),
@@ -1666,7 +1668,8 @@ fs.realpathSync = function realpathSync(p, options) {
16661668

16671669
// continue if not a symlink, break if a pipe/socket
16681670
if (knownHard[base] || (cache && cache.get(base) === base)) {
1669-
if (isFileType(S_IFIFO) || isFileType(S_IFSOCK)) {
1671+
if (isFileType(statValues, S_IFIFO) ||
1672+
isFileType(statValues, S_IFSOCK)) {
16701673
break;
16711674
}
16721675
continue;
@@ -1682,10 +1685,10 @@ fs.realpathSync = function realpathSync(p, options) {
16821685

16831686
var baseLong = pathModule.toNamespacedPath(base);
16841687
const ctx = { path: base };
1685-
binding.lstat(baseLong, undefined, ctx);
1688+
const stats = binding.lstat(baseLong, undefined, ctx);
16861689
handleErrorFromBinding(ctx);
16871690

1688-
if (!isFileType(S_IFLNK)) {
1691+
if (!isFileType(stats, S_IFLNK)) {
16891692
knownHard[base] = true;
16901693
if (cache) cache.set(base, base);
16911694
continue;
@@ -1696,8 +1699,8 @@ fs.realpathSync = function realpathSync(p, options) {
16961699
var linkTarget = null;
16971700
var id;
16981701
if (!isWindows) {
1699-
var dev = statValues[0].toString(32);
1700-
var ino = statValues[7].toString(32);
1702+
var dev = stats[0].toString(32);
1703+
var ino = stats[7].toString(32);
17011704
id = `${dev}:${ino}`;
17021705
if (seenLinks[id]) {
17031706
linkTarget = seenLinks[id];
@@ -1778,7 +1781,7 @@ fs.realpath = function realpath(p, options, callback) {
17781781

17791782
// On windows, check that the root exists. On unix there is no need.
17801783
if (isWindows && !knownHard[base]) {
1781-
fs.lstat(base, function(err) {
1784+
fs.lstat(base, function(err, stats) {
17821785
if (err) return callback(err);
17831786
knownHard[base] = true;
17841787
LOOP();
@@ -1811,7 +1814,8 @@ fs.realpath = function realpath(p, options, callback) {
18111814

18121815
// continue if not a symlink, break if a pipe/socket
18131816
if (knownHard[base]) {
1814-
if (isFileType(S_IFIFO) || isFileType(S_IFSOCK)) {
1817+
if (isFileType(statValues, S_IFIFO) ||
1818+
isFileType(statValues, S_IFSOCK)) {
18151819
return callback(null, encodeRealpathResult(p, options));
18161820
}
18171821
return process.nextTick(LOOP);
@@ -1820,14 +1824,11 @@ fs.realpath = function realpath(p, options, callback) {
18201824
return fs.lstat(base, gotStat);
18211825
}
18221826

1823-
function gotStat(err) {
1827+
function gotStat(err, stats) {
18241828
if (err) return callback(err);
18251829

1826-
// Use stats array directly to avoid creating an fs.Stats instance just for
1827-
// our internal use.
1828-
18291830
// if not a symlink, skip to the next path part
1830-
if (!isFileType(S_IFLNK)) {
1831+
if (!stats.isSymbolicLink()) {
18311832
knownHard[base] = true;
18321833
return process.nextTick(LOOP);
18331834
}
@@ -1837,8 +1838,8 @@ fs.realpath = function realpath(p, options, callback) {
18371838
// dev/ino always return 0 on windows, so skip the check.
18381839
let id;
18391840
if (!isWindows) {
1840-
var dev = statValues[0].toString(32);
1841-
var ino = statValues[7].toString(32);
1841+
var dev = stats.dev.toString(32);
1842+
var ino = stats.ino.toString(32);
18421843
id = `${dev}:${ino}`;
18431844
if (seenLinks[id]) {
18441845
return gotTarget(null, seenLinks[id], base);

‎lib/internal/fs.js‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ const {
3535
UV_FS_SYMLINK_DIR,
3636
UV_FS_SYMLINK_JUNCTION
3737
} = process.binding('constants').fs;
38-
const { statValues } = process.binding('fs');
3938

4039
const isWindows = process.platform === 'win32';
4140

@@ -217,10 +216,6 @@ function getStatsFromBinding(stats, offset = 0) {
217216
stats[12 + offset], stats[13 + offset]);
218217
}
219218

220-
function getStatsFromGlobalBinding(offset = 0) {
221-
return getStatsFromBinding(statValues, offset);
222-
}
223-
224219
function stringToFlags(flags) {
225220
if (typeof flags === 'number') {
226221
return flags;
@@ -442,7 +437,6 @@ module.exports = {
442437
preprocessSymlinkDestination,
443438
realpathCacheKey: Symbol('realpathCacheKey'),
444439
getStatsFromBinding,
445-
getStatsFromGlobalBinding,
446440
stringToFlags,
447441
stringToSymlinkType,
448442
Stats,

‎src/node_file.cc‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -410,8 +410,7 @@ void FSReqWrap::Reject(Local<Value> reject) {
410410
}
411411

412412
void FSReqWrap::ResolveStat(const uv_stat_t* stat) {
413-
node::FillGlobalStatsArray(env(), stat);
414-
Resolve(env()->fs_stats_field_array()->GetJSArray());
413+
Resolve(node::FillGlobalStatsArray(env(), stat));
415414
}
416415

417416
void FSReqWrap::Resolve(Local<Value> value) {
@@ -832,10 +831,13 @@ static void Stat(const FunctionCallbackInfo<Value>& args) {
832831
FS_SYNC_TRACE_BEGIN(stat);
833832
int err = SyncCall(env, args[2], &req_wrap_sync, "stat", uv_fs_stat, *path);
834833
FS_SYNC_TRACE_END(stat);
835-
if (err == 0) {
836-
node::FillGlobalStatsArray(env,
837-
static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
834+
if (err != 0) {
835+
return; // error info is in ctx
838836
}
837+
838+
Local<Value> arr = node::FillGlobalStatsArray(env,
839+
static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
840+
args.GetReturnValue().Set(arr);
839841
}
840842
}
841843

@@ -859,10 +861,13 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
859861
int err = SyncCall(env, args[2], &req_wrap_sync, "lstat", uv_fs_lstat,
860862
*path);
861863
FS_SYNC_TRACE_END(lstat);
862-
if (err == 0) {
863-
node::FillGlobalStatsArray(env,
864-
static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
864+
if (err != 0) {
865+
return; // error info is in ctx
865866
}
867+
868+
Local<Value> arr = node::FillGlobalStatsArray(env,
869+
static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
870+
args.GetReturnValue().Set(arr);
866871
}
867872
}
868873

@@ -885,10 +890,13 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
885890
FS_SYNC_TRACE_BEGIN(fstat);
886891
int err = SyncCall(env, args[2], &req_wrap_sync, "fstat", uv_fs_fstat, fd);
887892
FS_SYNC_TRACE_END(fstat);
888-
if (err == 0) {
889-
node::FillGlobalStatsArray(env,
890-
static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
893+
if (err != 0) {
894+
return; // error info is in ctx
891895
}
896+
897+
Local<Value> arr = node::FillGlobalStatsArray(env,
898+
static_cast<const uv_stat_t*>(req_wrap_sync.req.ptr));
899+
args.GetReturnValue().Set(arr);
892900
}
893901
}
894902

‎src/node_internals.h‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
313313
const char* deprecation_code);
314314

315315
template <typename NativeT, typename V8T>
316-
void FillStatsArray(AliasedBuffer<NativeT, V8T>* fields_ptr,
316+
v8::Local<v8::Value> FillStatsArray(AliasedBuffer<NativeT, V8T>* fields_ptr,
317317
const uv_stat_t* s, int offset = 0) {
318318
AliasedBuffer<NativeT, V8T>& fields = *fields_ptr;
319319
fields[offset + 0] = s->st_dev;
@@ -347,12 +347,14 @@ void FillStatsArray(AliasedBuffer<NativeT, V8T>* fields_ptr,
347347
X(12, ctim)
348348
X(13, birthtim)
349349
#undef X
350+
351+
return fields_ptr->GetJSArray();
350352
}
351353

352-
inline void FillGlobalStatsArray(Environment* env,
353-
const uv_stat_t* s,
354-
int offset = 0) {
355-
node::FillStatsArray(env->fs_stats_field_array(), s, offset);
354+
inline v8::Local<v8::Value> FillGlobalStatsArray(Environment* env,
355+
const uv_stat_t* s,
356+
int offset = 0) {
357+
return node::FillStatsArray(env->fs_stats_field_array(), s, offset);
356358
}
357359

358360
void SetupProcessObject(Environment* env,

‎src/node_stat_watcher.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,12 @@ void StatWatcher::Callback(uv_fs_poll_t* handle,
108108
HandleScope handle_scope(env->isolate());
109109
Context::Scope context_scope(env->context());
110110

111-
node::FillGlobalStatsArray(env, curr);
111+
Local<Value> arr = node::FillGlobalStatsArray(env, curr);
112112
node::FillGlobalStatsArray(env, prev, env->kFsStatsFieldsLength);
113113

114114
Local<Value> argv[2] {
115115
Integer::New(env->isolate(), status),
116-
env->fs_stats_field_array()->GetJSArray()
116+
arr
117117
};
118118
wrap->MakeCallback(env->onchange_string(), arraysize(argv), argv);
119119
}

0 commit comments

Comments
 (0)