fs: support BigInt in fs.*stat and fs.watchFile #20220

joyeecheung · Apr 23, 2018

fs: support BigInt in fs.*stat and fs.watchFile
Add the bigint: true option to all the fs.*stat methods and
fs.watchFile.
doc: document BigInt support in fs.Stats

I chose to expose the BigInt variant via bigint: true in the option object because right now there are no support of a Date class with higher precision - the old Date is Number-based and can lose precision when being constructed out of a 64bit ms value. The plan is that when JS does get a better Date e.g. Temporal (see tc39/proposal-bigint#136), we will be able to support temporal: true.

The --harmony-bigint flag is necessary until v8 6.7 lands.

Fixes: #12115

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

vsemozhetbyt · Apr 23, 2018

vsemozhetbyt reviewed Apr 23, 2018

View changes

Sorry for many repetitive nits, they are guards to not be missed. I will delete them if they will be rejected or not folded when addressed.

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

@@ -413,6 +413,8 @@ A `fs.Stats` object provides information about a file.

 Objects returned from [`fs.stat()`][], [`fs.lstat()`][] and [`fs.fstat()`][] and
 their synchronous counterparts are of this type.
+If `bigint` in the `options` passed to those methods is true, the numeric values
+will be `BigInt` instead of `Number`.


If we mean primitives and not the classes/object wrappers, should these be bigint and number?

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

@@ -436,6 +438,30 @@ Stats {
  birthtime: Mon, 10 Oct 2011 23:24:11 GMT }
 ```

+`BigInt` version:


Ditto, `bigint`?

vsemozhetbyt · Apr 23, 2018

tools/doc/type-parser.js

@@ -4,6 +4,7 @@ const jsDocPrefix = 'https://developer.mozilla.org/en-US/docs/Web/JavaScript/';

 const jsDataStructuresUrl = `${jsDocPrefix}Data_structures`;
 const jsPrimitives = {
+  bigint: 'BigInt',


Unfortunately, this will generate:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#BigInt_type

due to this end point, and there is no such section in the MDN. Unless this section is created by our collaborators, we need temporarily place this type along with something like AsyncIterator in customTypesMap, before Node.js types (as 'bigint' after 'AsyncIterator') with a link to the proposal.

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

@@ -1636,9 +1662,16 @@ changes:
    pr-url: https://github.com/nodejs/node/pull/7897
    description: The `callback` parameter is no longer optional. Not passing
                 it will emit a deprecation warning with id DEP0013.
+  - version: REPLACEME
+    pr-url: REPLACEME
+    description: Accepts an additional options object to specify whether


options -> `options`?

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

 -->

 * `fd` {integer}
+* `options` {Object}
+  * `bigint` {boolean} **Default:** `false` Whether the numeric values in


Default values usually go at the end, with a period if preceded by a full sentence or if constructed as a full sentence. So this should be:

...should be `BigInt`. **Default:** `false`.

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

 -->

 * `path` {string|Buffer|URL}
+* `options` {Object}
+  * `bigint` {boolean} **Default:** `false` Whether the numeric values in


Ditto re default value place and format.

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

 -->

 * `path` {string|Buffer|URL}
+* `options` {Object}
+  * `bigint` {boolean} **Default:** `false` Whether the numeric values in
+    the returned [`fs.Stats`][] object should be `BigInt`.


`bigint` as a primitive type?

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

+changes:
+  - version: REPLACEME
+    pr-url: REPLACEME
+    description: Accepts an additional options object to specify whether


options -> `options`?

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

 -->

 * `path` {string|Buffer|URL}
+* `options` {Object}
+  * `bigint` {boolean} **Default:** `false` Whether the numeric values in


Ditto re default value place and format.

vsemozhetbyt · Apr 23, 2018

doc/api/fs.md

 -->

 * `path` {string|Buffer|URL}
+* `options` {Object}
+  * `bigint` {boolean} **Default:** `false` Whether the numeric values in
+    the returned [`fs.Stats`][] object should be `BigInt`.


`bigint` as a primitive type?

BridgeAR · Apr 23, 2018

BridgeAR reviewed Apr 23, 2018

View changes

BridgeAR · Apr 23, 2018

test/parallel/test-fs-stat-bigint.js

+  const numStats = await handle.stat();
+  verifyStats(bigintStats, numStats);
+  await handle.close();
+})();


These tests would all pass in case e.g. const handle = await promiseFs.open(fn, 'r'); rejects. In that case it would log an unhandled rejection and that would currently not end the process, so we would not notice anything wrong.

Instead, it would be good to write something like:

async function foo() { } Promise.all([ foo(), bar(), ... ]).then(common.mustCall());

There is also a common function to throw in case of unhandled rejections.

I went with common.crashOnUnhandledRejection() because it's simple

joyeecheung · Jun 4, 2018

I think I've addressed all the comments from @vsemozhetbyt and @BridgeAR . This should be ready now that v8 6.7 has landed on master. PTAL @nodejs/fs

CI: https://ci.nodejs.org/job/node-test-pull-request/15249/

bnoordhuis · Jun 5, 2018

Looks like lib/internal/fs/promises.js is missing this:

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 43956dae3f..a1670fc65b 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -476,6 +476,7 @@ module.exports = {
   symlink,
   lstat,
   stat,
+  fstat,
   link,
   unlink,
   chmod,

joyeecheung · Jun 5, 2018

@bnoordhuis fsPromises.fstat should already be removed by #20559 , I forgot to update the tests. I'll update it.

joyeecheung · Jun 5, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/15257/

bnoordhuis · Jun 5, 2018

Still saying TypeError: promiseFs.fstat is not a function.

joyeecheung · Jun 5, 2018

https://ci.nodejs.org/job/node-test-pull-request/15260/

Oops, forgot to push

bnoordhuis · Jun 5, 2018

Still failing on Windows, I'm afraid: https://ci.nodejs.org/job/node-test-binary-windows/17813/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=2/console

joyeecheung · Jun 5, 2018

Green on my Windows laptop now. @bnoordhuis Would you mind taking a look at the last commit? I think it should be OK to special case for Windows like that.

Another CI: https://ci.nodejs.org/job/node-test-pull-request/15266/

joyeecheung · Jun 5, 2018

joyeecheung reviewed Jun 5, 2018

View changes

joyeecheung · Jun 5, 2018

lib/internal/fs/utils.js

@@ -155,6 +155,10 @@ function Stats(
 }

 Stats.prototype._checkModeProperty = function(property) {
+  if (isWindows && (property === S_IFIFO || property === S_IFBLK ||
+    property === S_IFSOCK)) {


These three are in fact all undefined on Windows, but I figured it looks more readable if I list them like that.

bnoordhuis · Jun 5, 2018

bnoordhuis approved these changes Jun 5, 2018

View changes

bnoordhuis · Jun 5, 2018

src/node_internals.h

-  return node::FillStatsArray(env->fs_stats_field_array(), s, offset);
+                                          const uv_stat_t* s,
+                                          bool use_bigint = false,
+                                          int offset = 0) {


Alignment is off.

bnoordhuis · Jun 5, 2018

src/node_stat_watcher.h

@@ -57,6 +57,7 @@ class StatWatcher : public AsyncWrap {
  bool IsActive();

  uv_fs_poll_t* watcher_;
+  bool use_bigint_;


const bool?

joyeecheung · Jun 6, 2018

Test failures seem to be unrelated:

Failures in job https://ci.nodejs.org/job/node-test-commit/18998/

alpine-latest-x64

See failures

not ok 2193 sequential/test-inspector-port-zero-cluster
  ---
  duration_ms: 0.413
  severity: fail
  exitcode: 1
  stack: |-
    Debugger listening on ws://127.0.0.1:39191/3e6681d2-89d9-4b27-989b-baed7a6a650b
    For help, see: https://nodejs.org/en/docs/inspector
    Starting inspector on 127.0.0.1:39192 failed: address already in use
    Assertion failed: !uv__is_closing(handle) (../deps/uv/src/unix/core.c: uv_close: 117)
    Debugger listening on ws://127.0.0.1:39193/708697be-39f0-4229-96b0-354e4b76ccca
    For help, see: https://nodejs.org/en/docs/inspector
    Starting inspector on 127.0.0.1:39194 failed: address already in use
    Assertion failed: !uv__is_closing(handle) (../deps/uv/src/unix/core.c: uv_close: 117)
    assert.js:80
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: code: null, signal: SIGABRT
        at Worker.worker.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-latest-x64/test/sequential/test-inspector-port-zero-cluster.js:20:16)
        at Worker.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-latest-x64/test/common/index.js:443:15)
        at Worker.emit (events.js:182:13)
        at ChildProcess.worker.process.once (internal/cluster/master.js:193:12)
        at Object.onceWrapper (events.js:273:13)
        at ChildProcess.emit (events.js:182:13)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:237:12)
  ...

fedora-latest-x64

See failures

error: argument --input/-i: can't open 'test.tap': [Errno 2] No such file or directory: 'test.tap'
POST BUILD TASK : FAILURE
END OF POST BUILD TASK : 0
Recording test results
Notifying upstream projects of job completion
Finished: FAILURE

centos7-arm64-gcc6

See failures

not ok 2246 sequential/test-inspector-port-zero-cluster
  ---
  duration_ms: 0.332
  severity: fail
  exitcode: 1
  stack: |-
    Debugger listening on ws://127.0.0.1:34395/ed174642-2254-4d6e-83e7-e18fceb7ae24
    For help, see: https://nodejs.org/en/docs/inspector
    Starting inspector on 127.0.0.1:34396 failed: address already in use
    node: ../deps/uv/src/unix/core.c:117: uv_close: Assertion `!(((handle)->flags & (UV_CLOSING | UV_CLOSED)) != 0)' failed.
    Debugger listening on ws://127.0.0.1:34397/8f93d94c-ae7b-4067-806f-f145561dcf5c
    For help, see: https://nodejs.org/en/docs/inspector
    Debugger listening on ws://127.0.0.1:34398/a6c6b38b-7b83-4a86-9897-279ca8d6d6e3
    For help, see: https://nodejs.org/en/docs/inspector
    assert.js:80
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: code: null, signal: SIGABRT
        at Worker.worker.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-arm/nodes/centos7-arm64-gcc6/test/sequential/test-inspector-port-zero-cluster.js:20:16)
        at Worker.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/nodes/centos7-arm64-gcc6/test/common/index.js:443:15)
        at Worker.emit (events.js:182:13)
        at ChildProcess.worker.process.once (internal/cluster/master.js:193:12)
        at Object.onceWrapper (events.js:273:13)
        at ChildProcess.emit (events.js:182:13)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:237:12)
  ...

debian7-docker-armv7: Unknown

joyeecheung · Jun 6, 2018

Addressed nits. New CI: https://ci.nodejs.org/job/node-test-pull-request/15277/

joyeecheung · Jun 6, 2018

CI is green. Pinging @nodejs/fs again for more reviews.

joyeecheung · Jun 7, 2018

I plan to land this later today and try backporting it after #21172 lands.

joyeecheung · Jun 7, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15310/

joyeecheung · Jun 7, 2018

Failures seem to be unrelated:

Failures in job https://ci.nodejs.org/job/node-test-commit/19053/

centos7-64-gcc6

See failures

not ok 2179 sequential/test-inspector-port-zero-cluster
  ---
  duration_ms: 0.280
  severity: fail
  exitcode: 1
  stack: |-
    Debugger listening on ws://127.0.0.1:37752/8c04df0a-cdec-4543-b2fa-2ec22a3495df
    For help, see: https://nodejs.org/en/docs/inspector
    Debugger listening on ws://127.0.0.1:37753/fcae2cf6-2517-4b80-8c02-1341177bc72b
    For help, see: https://nodejs.org/en/docs/inspector
    Starting inspector on 127.0.0.1:37754 failed: address already in use
    node: ../deps/uv/src/unix/core.c:117: uv_close: Assertion `!(((handle)->flags & (UV_CLOSING | UV_CLOSED)) != 0)' failed.
    assert.js:80
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: code: null, signal: SIGABRT
        at Worker.worker.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos7-64-gcc6/test/sequential/test-inspector-port-zero-cluster.js:20:16)
        at Worker.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos7-64-gcc6/test/common/index.js:451:15)
        at Worker.emit (events.js:182:13)
        at ChildProcess.worker.process.once (internal/cluster/master.js:193:12)
        at Object.onceWrapper (events.js:273:13)
        at ChildProcess.emit (events.js:182:13)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:237:12)
    Debugger listening on ws://127.0.0.1:37755/61ef8c5b-1ab7-41d2-a76e-e0aa9ec6194e
    For help, see: https://nodejs.org/en/docs/inspector
  ...

ubuntu1604_sharedlibs_openssl110_x64

See failures

not ok 1961 parallel/test-worker-dns-terminate
  ---
  duration_ms: 0.985
  severity: crashed
  exitcode: -11
  stack: |-
  ...

nodejs-github-bot added C++ lib / src labels Apr 23, 2018

mscdex added the work in progress (WIP) label Apr 23, 2018

joyeecheung force-pushed the joyeecheung:bigint-stat branch from d7db80b to fa59747 Jun 2, 2018

joyeecheung changed the title ~~WIP: fs: support BigInt in fs.*stat and fs.watchFile~~ fs: support BigInt in fs.*stat and fs.watchFile Jun 4, 2018

joyeecheung removed the work in progress (WIP) label Jun 5, 2018

joyeecheung force-pushed the joyeecheung:bigint-stat branch from fa59747 to 17fda05 Jun 5, 2018

fixup! fs: support BigInt in fs.*stat and fs.watchFile

Verified

This commit was signed with a verified signature.

joyeecheung Joyee Cheung

GPG key ID: 92B78A53C8303B8D Learn about signing commits

Loading status checks…

3caab98

joyeecheung force-pushed the joyeecheung:bigint-stat branch from 9642218 to 3caab98 Jun 5, 2018

fixup! fs: support BigInt in fs.*stat and fs.watchFile

Verified

This commit was signed with a verified signature.

joyeecheung Joyee Cheung

GPG key ID: 92B78A53C8303B8D Learn about signing commits

Loading status checks…

dc18efd

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

doc: document BigInt support in fs.Stats
PR-URL: nodejs#20220 Fixes: nodejs#12115 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

12ace57

seishun referenced this pull request Jun 9, 2018
Closed
tools: add BigInt to globals #21237
2 of 2 tasks complete

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

doc: document BigInt support in fs.Stats
PR-URL: nodejs#20220 Fixes: nodejs#12115 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

082703c

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

doc: document BigInt support in fs.Stats
PR-URL: nodejs#20220 Fixes: nodejs#12115 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

b2b40bf

targos added a commit that referenced this pull request Jun 13, 2018

fs: support BigInt in fs.*stat and fs.watchFile
Add the `bigint: true` option to all the `fs.*stat` methods and `fs.watchFile`. PR-URL: #20220 Fixes: #12115 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

04e8f07

targos added a commit that referenced this pull request Jun 13, 2018

doc: document BigInt support in fs.Stats
PR-URL: #20220 Fixes: #12115 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

fe5d351

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

doc: document BigInt support in fs.Stats
PR-URL: nodejs#20220 Fixes: nodejs#12115 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

863eba7

targos referenced this pull request Jun 19, 2018
Merged
v10.5.0 proposal #21400

This was referenced Jun 20, 2018

Closed

2018-06-20 Version 10.5.0 (Current) @targos beevelop/docker-android-nodejs#216

Closed

2018-06-20 Version 10.5.0 (Current) @targos beevelop/docker-nodejs#201

wmertens referenced this pull request Jul 9, 2018
Open
stat.ino can be truncated #4

MylesBorins added dont-land-on-v6.x dont-land-on-v8.x labels Aug 10, 2018

pixtron referenced this pull request Nov 7, 2018
Open
Upgrade to electron v4.x #132

bnoordhuis referenced this pull request Feb 4, 2019
Closed
on Win10, fs.stat() returns a stats with undefined blksize and blocks #25913

nodejs/node

Join GitHub today

fs: support BigInt in fs.*stat and fs.watchFile #20220

Conversation

joyeecheung commented Apr 23, 2018 • edited

Checklist

nodejs-github-bot added C++ lib / src labels Apr 23, 2018

mscdex added the work in progress (WIP) label Apr 23, 2018

vsemozhetbyt reviewed Apr 23, 2018 View changes

vsemozhetbyt left a comment

This comment has been minimized.

This comment has been minimized.

vsemozhetbyt Apr 23, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

BridgeAR reviewed Apr 23, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joyeecheung force-pushed the joyeecheung:bigint-stat branch from d7db80b to fa59747 Jun 2, 2018

joyeecheung changed the title WIP: fs: support BigInt in fs.*stat and fs.watchFile fs: support BigInt in fs.*stat and fs.watchFile Jun 4, 2018

This comment has been minimized.

joyeecheung commented Jun 4, 2018

This comment has been minimized.

bnoordhuis commented Jun 5, 2018

This comment has been minimized.

joyeecheung commented Jun 5, 2018

joyeecheung removed the work in progress (WIP) label Jun 5, 2018

This comment has been minimized.

joyeecheung commented Jun 5, 2018

This comment has been minimized.

bnoordhuis commented Jun 5, 2018

joyeecheung added some commits Apr 7, 2018

joyeecheung force-pushed the joyeecheung:bigint-stat branch from fa59747 to 17fda05 Jun 5, 2018

This comment has been minimized.

joyeecheung commented Jun 5, 2018

This comment has been minimized.

bnoordhuis commented Jun 5, 2018

joyeecheung force-pushed the joyeecheung:bigint-stat branch from 9642218 to 3caab98 Jun 5, 2018

This comment has been minimized.

joyeecheung commented Jun 5, 2018 • edited

joyeecheung reviewed Jun 5, 2018 View changes

This comment has been minimized.

bnoordhuis approved these changes Jun 5, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joyeecheung commented Jun 6, 2018

debian7-docker-armv7: Unknown

This comment has been minimized.

joyeecheung commented Jun 6, 2018

This comment has been minimized.

joyeecheung commented Jun 6, 2018

This comment has been minimized.

joyeecheung commented Jun 7, 2018

This comment has been minimized.

joyeecheung commented Jun 7, 2018

This comment has been minimized.

joyeecheung commented Jun 7, 2018 • edited

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

seishun referenced this pull request Jun 9, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

targos added a commit that referenced this pull request Jun 13, 2018

targos added a commit that referenced this pull request Jun 13, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

targos referenced this pull request Jun 19, 2018

targos added a commit that referenced this pull request Jun 20, 2018

targos added a commit that referenced this pull request Jun 20, 2018

targos added a commit that referenced this pull request Jun 20, 2018

This was referenced Jun 20, 2018

joyeecheung commented Apr 23, 2018 •

edited

vsemozhetbyt reviewed Apr 23, 2018

View changes

vsemozhetbyt Apr 23, 2018 •

edited

BridgeAR reviewed Apr 23, 2018

View changes

joyeecheung force-pushed the joyeecheung:bigint-stat branch from `d7db80b` to `fa59747` Jun 2, 2018

joyeecheung changed the title WIP: fs: support BigInt in fs.stat and fs.watchFile fs: support BigInt in fs.stat and fs.watchFile Jun 4, 2018

joyeecheung force-pushed the joyeecheung:bigint-stat branch from `fa59747` to `17fda05` Jun 5, 2018

joyeecheung force-pushed the joyeecheung:bigint-stat branch from `9642218` to `3caab98` Jun 5, 2018

joyeecheung commented Jun 5, 2018 •

edited

joyeecheung reviewed Jun 5, 2018

View changes

bnoordhuis approved these changes Jun 5, 2018

View changes

joyeecheung commented Jun 7, 2018 •

edited