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

joyeecheung · 2018-04-23T09:57:39Z

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

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 · 2018-04-23T13:18:42Z

doc/api/fs.md

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

vsemozhetbyt · 2018-04-23T13:20:52Z

doc/api/fs.md

Ditto, `bigint`?

vsemozhetbyt · 2018-04-23T13:30:54Z

tools/doc/type-parser.js

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 · 2018-04-23T13:38:43Z

doc/api/fs.md

options -> `options`?

vsemozhetbyt · 2018-04-23T13:44:17Z

doc/api/fs.md

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 · 2018-04-23T14:14:36Z

doc/api/fs.md

Ditto re default value place and format.

vsemozhetbyt · 2018-04-23T14:14:51Z

doc/api/fs.md

`bigint` as a primitive type?

vsemozhetbyt · 2018-04-23T14:16:20Z

doc/api/fs.md

options -> `options`?

vsemozhetbyt · 2018-04-23T14:16:39Z

doc/api/fs.md

Ditto re default value place and format.

vsemozhetbyt · 2018-04-23T14:16:49Z

doc/api/fs.md

`bigint` as a primitive type?

BridgeAR · 2018-04-23T14:55:49Z

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

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 · 2018-06-04T15:43:42Z

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 · 2018-06-05T11:18:56Z

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 · 2018-06-05T12:48:37Z

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

joyeecheung · 2018-06-05T12:57:40Z

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

bnoordhuis · 2018-06-05T14:16:09Z

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

Add the `bigint: true` option to all the `fs.*stat` methods and `fs.watchFile`.

joyeecheung · 2018-06-05T15:26:33Z

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

Oops, forgot to push

bnoordhuis · 2018-06-05T17:58:27Z

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 · 2018-06-05T19:04:28Z

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 · 2018-06-05T19:07:20Z

lib/internal/fs/utils.js


 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 · 2018-06-05T19:16:15Z

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 · 2018-06-05T19:17:06Z

src/node_stat_watcher.h

  bool IsActive();

  uv_fs_poll_t* watcher_;
+  bool use_bigint_;


const bool?

joyeecheung · 2018-06-06T06:00:00Z

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 · 2018-06-06T06:19:38Z

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

joyeecheung · 2018-06-06T13:03:41Z

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

joyeecheung · 2018-06-07T09:06:25Z

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

joyeecheung · 2018-06-07T09:24:20Z

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

joyeecheung · 2018-06-07T13:48:15Z

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++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 23, 2018

mscdex added the wip Issues and PRs that are still a work in progress. label Apr 23, 2018

vsemozhetbyt reviewed Apr 23, 2018

View reviewed changes

BridgeAR reviewed Apr 23, 2018

View reviewed changes

joyeecheung force-pushed the bigint-stat branch from d7db80b to fa59747 Compare June 2, 2018 16:39

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 wip Issues and PRs that are still a work in progress. label Jun 5, 2018

joyeecheung added 2 commits June 5, 2018 23:25

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

4ee0c9c

Add the `bigint: true` option to all the `fs.*stat` methods and `fs.watchFile`.

doc: document BigInt support in fs.Stats

17fda05

joyeecheung force-pushed the bigint-stat branch from fa59747 to 17fda05 Compare June 5, 2018 15:25

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

3caab98

joyeecheung force-pushed the bigint-stat branch from 9642218 to 3caab98 Compare June 5, 2018 17:58

joyeecheung commented Jun 5, 2018

View reviewed changes

bnoordhuis approved these changes Jun 5, 2018

View reviewed changes

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

dc18efd

seishun mentioned this pull request Jun 9, 2018

tools: add BigInt to globals #21237

Closed

2 tasks

targos mentioned this pull request Jun 19, 2018

v10.5.0 proposal #21400

Merged

wmertens mentioned this pull request Jul 9, 2018

stat.ino can be truncated mixu/file-dedupe#4

Open

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

pixtron mentioned this pull request Nov 7, 2018

Upgrade to electron v7 gyselroth/balloon-client-desktop#132

Open

bnoordhuis mentioned this pull request Feb 4, 2019

on Win10, fs.stat() returns a stats with undefined blksize and blocks #25913

Closed

weswigham mentioned this pull request Mar 5, 2020

Solid Node.js with official type system support #32022

Closed

Uh oh!

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

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

Uh oh!

Conversation

joyeecheung commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jun 4, 2018

Uh oh!

bnoordhuis commented Jun 5, 2018

Uh oh!

joyeecheung commented Jun 5, 2018

Uh oh!

joyeecheung commented Jun 5, 2018

Uh oh!

bnoordhuis commented Jun 5, 2018

Uh oh!

joyeecheung commented Jun 5, 2018

Uh oh!

bnoordhuis commented Jun 5, 2018

Uh oh!

joyeecheung commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jun 6, 2018

debian7-docker-armv7: Unknown

Uh oh!

joyeecheung commented Jun 6, 2018

Uh oh!

joyeecheung commented Jun 6, 2018

Uh oh!

joyeecheung commented Jun 7, 2018

Uh oh!

joyeecheung commented Jun 7, 2018

Uh oh!

joyeecheung commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

joyeecheung commented Apr 23, 2018 •

edited

Loading

vsemozhetbyt Apr 23, 2018 •

edited

Loading

joyeecheung commented Jun 5, 2018 •

edited

Loading

joyeecheung commented Jun 7, 2018 •

edited

Loading