fs: add *timeNs properties to BigInt Stats objects by joyeecheung · Pull Request #21387 · nodejs/node · GitHub

Extend the aliased buffer for stats objects to contain
the entire time spec (seconds and nanoseconds) for the time
values instead of calculating the milliseconds in C++ and
lose precision.
Calculate the nanosecond-precision time values in JS and expose
them in BigInt Stats objects as *timeNs. The
millisecond-precision values are now calculated from the
nanosecond-precision values.
Refactor the hierarchy of stats objects and use ES6 classes.

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

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/91/pipeline
CI: https://ci.nodejs.org/job/node-test-pull-request/23863/

cc @nodejs/fs
CI: https://ci.nodejs.org/job/node-test-pull-request/15512/

I like it. One nit.

else would a touch clearer

@Fishrock123 I think we prefer leaving out the else block for early returns? There used to a PR adding a lint rule for that although it didn't land due to the amount of churn.

While I'm good with this, changing fs.Stats to a ES6 class is necessarily a semver-major change because fs.Stats() is no longer callable without new.

Just a thought... we may want to consider adding a custom toJSON() function to the BitIntStats object to avoid...

> JSON.stringify(fs.statSync('.', { bigint: true }))
TypeError: Do not know how to serialize a BigInt
    at JSON.stringify (<anonymous>)

JSON.stringify(fs.statSync('.', { bigint: true }))
TypeError: Do not know how to serialize a BigInt
at JSON.stringify ()

@jasnell hm, thanks for raising that up, but we need to be careful about the format we pick since people may store them into DB or send them to clients that are not implemented in JavaScript...

On a second thought, the ES6 classes probably does not worth making this semver major. I have changed it back to the ES5 way of doing things. @jasnell

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

EDIT: forgot to push. New CI https://ci.nodejs.org/job/node-test-pull-request/15645/

Ping @jasnell does this look good to you now as a non-semver-major?

Yes, looks good

Previous CI is inaccessible, new CI: https://ci.nodejs.org/job/node-test-pull-request/15855/

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

Looks like it broke something related to #13255 on Windows, I'll investigate.

Failures in job https://ci.nodejs.org/job/node-test-pull-request/15920/

debian8-x86

See failures on test-softlayer-debian8-x86-1:

not ok 658 parallel/test-fs-utimes
  ---
  duration_ms: 0.331
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/debian8-x86/test/parallel/test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

ubuntu1404-32

See failures on test-digitalocean-ubuntu1404-x86-1:

not ok 601 parallel/test-fs-utimes
  ---
  duration_ms: 0.629
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32/test/parallel/test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

ubuntu1604-32

See failures on test-digitalocean-ubuntu1604-x86-2:

not ok 658 parallel/test-fs-utimes
  ---
  duration_ms: 0.171
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1604-32/test/parallel/test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

COMPILED_BY=vs2017,RUNNER=win10,RUN_SUBSET=3

See failures on test-azure_msft-win10-x64-3:

not ok 165 parallel/test-fs-utimes
  ---
  duration_ms: 0.121
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=3

See failures on test-rackspace-win2008r2-x64-3:

not ok 165 parallel/test-fs-utimes
  ---
  duration_ms: 0.198
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=3

See failures on test-azure_msft-win2016-x64-3:

not ok 165 parallel/test-fs-utimes
  ---
  duration_ms: 0.120
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

COMPILED_BY=vs2017-x86,RUNNER=win2012r2,RUN_SUBSET=3

See failures on test-rackspace-win2012r2-x64-1:

not ok 165 parallel/test-fs-utimes
  ---
  duration_ms: 0.203
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
    + expected - actual
    
    - 2147483648
    + -2147483647.999
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-fs-utimes.js:196:10)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:266:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:596:3)
  ...

Won't be able to be around a machine to look into this this week

What's the status on this one?

Can we try fixing merge conflicts and running the tests again? I'd be happy to give it a shot in another pull request

- Extend the aliased buffer for stats objects to contain the entire time spec (seconds and nanoseconds) for the time values instead of calculating the milliseconds in C++ and lose precision there. - Calculate the nanosecond-precision time values in JS and expose them in BigInt Stats objects as `*timeNs`. The millisecond-precision values are now calculated from the nanosecond-precision values.

Resume CI: https://ci.nodejs.org/job/node-test-commit/29357/

We can't pass a time spec into the Stats constructor as that would be a breaking change since Stats is public. We may try using some factory function to construct Stats internally from time specs later, and try adding the Ns properties to normal Stats later, though, but there will be some precision losses without bigint and that's not too different from multiplying the Ms ones with 1000

The CI is now green (minus an unrelated failure #27611 )

@jasnell @Fishrock123 can you please take a look again?

Also @devsnek this might be useful for #27850 , whichever lands first..

This should be changed, right?

That'll only be available when this is released.

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

Landed in b245257

- Extend the aliased buffer for stats objects to contain the entire time spec (seconds and nanoseconds) for the time values instead of calculating the milliseconds in C++ and lose precision there. - Calculate the nanosecond-precision time values in JS and expose them in BigInt Stats objects as `*timeNs`. The millisecond-precision values are now calculated from the nanosecond-precision values. PR-URL: #21387 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

Notable changes: * deps: * Update npm to 6.10.3 (isaacs) #29023 * fs: * Add recursive option to rmdir() (cjihrig) #29168 * Allow passing true to emitClose option (Giorgos Ntemiris) #29212 * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung) #21387 * net: * Allow reading data into a static buffer (Brian White) #25436 PR-URL: #29429

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Jun 18, 2018

vsemozhetbyt reviewed Jun 19, 2018

View reviewed changes

Comment thread doc/api/fs.md Outdated

Comment thread doc/api/fs.md Outdated

Fishrock123 approved these changes Jun 20, 2018

View reviewed changes

jasnell approved these changes Jun 21, 2018

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 21, 2018

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 26, 2018

joyeecheung removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 27, 2018

joyeecheung force-pushed the bigint-ns branch from 7c8170c to a11d4c7 Compare June 27, 2018 05:23

BridgeAR removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 18, 2018

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

joyeecheung force-pushed the bigint-ns branch from a11d4c7 to 0410dd2 Compare June 10, 2019 17:13

joyeecheung force-pushed the bigint-ns branch from aa034a6 to 06dff68 Compare June 10, 2019 20:40

joyeecheung removed the stalled Issues and PRs that are stalled. label Jun 11, 2019

joyeecheung commented Jun 11, 2019

View reviewed changes

joyeecheung mentioned this pull request Jun 11, 2019

Test /parallel/test-fs-stat-bigint fails 3 out of 100 times #24593

Closed

joyeecheung added the review wanted PRs that need reviews. label Jun 12, 2019

bjorn3 reviewed Jun 12, 2019

View reviewed changes

devsnek approved these changes Jun 12, 2019

View reviewed changes

joyeecheung closed this Jun 13, 2019

joyeecheung added semver-minor PRs that contain new features and should be released in the next minor version. and removed review wanted PRs that need reviews. labels Jun 13, 2019

joyeecheung mentioned this pull request Jun 16, 2019

CI failures: 20190612-20190615 nodejs/reliability#23

Closed

BridgeAR mentioned this pull request Jun 17, 2019

v12.5.0 proposal #28268

Merged

targos mentioned this pull request Jun 18, 2019

esm is broken by change in Node.js standard-things/esm#821

Open

targos added the backport-blocked-v12.x label Jul 2, 2019

targos mentioned this pull request Aug 19, 2019

fs: fix broken esm #28957

Closed

2 tasks

BridgeAR mentioned this pull request Sep 3, 2019

v12.10.0 proposal #29429

Merged

BridgeAR added backported-to-v12.x and removed backport-blocked-v12.x labels Sep 3, 2019

yuraxdrumz mentioned this pull request Oct 11, 2019

Time set incorrectly in Node 12 tschaub/mock-fs#276

Closed

+                  stats[3 + offset], stats[4 + offset], stats[5 + offset],
+                  stats[6 + offset], stats[7 + offset], stats[8 + offset],
+                  stats[9 + offset],
+                  msFromTimeSpec(stats[10 + offset], stats[11 + offset]),

+              ### stats.atimeNs
+              <!-- YAML
+              added: REPLACEME

fs: add *timeNs properties to BigInt Stats objects by joyeecheung · Pull Request #21387 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: add *timeNs properties to BigInt Stats objects - #21387

fs: add *timeNs properties to BigInt Stats objects#21387joyeecheung wants to merge 1 commit intonodejs:masternodejs/node:masterfrom joyeecheung:bigint-nsjoyeecheung/node:bigint-nsCopy head branch name to clipboard

Conversation

joyeecheung commented Jun 18, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Jun 18, 2018

Uh oh!

Uh oh!

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jun 27, 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!

jasnell commented Jun 21, 2018

Uh oh!

jasnell commented Jun 22, 2018

Uh oh!

joyeecheung commented Jun 22, 2018

Uh oh!

joyeecheung commented Jun 27, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jasnell commented Jul 10, 2018

Uh oh!

addaleax commented Jul 13, 2018

Uh oh!

Trott commented Jul 18, 2018

Uh oh!

joyeecheung commented Jul 18, 2018

Uh oh!

joyeecheung commented Jul 23, 2018

Uh oh!

jasnell commented Sep 10, 2018

Uh oh!

josepharhar commented Jan 21, 2019

Uh oh!

joyeecheung commented Jun 11, 2019

Uh oh!

joyeecheung Jun 11, 2019 • 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!

joyeecheung commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jun 13, 2019

Uh oh!

joyeecheung commented Jun 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

fs: add *timeNs properties to BigInt Stats objects#21387
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:bigint-ns

nodejs-github-bot commented Jun 18, 2018 •
edited
Loading

joyeecheung Jun 27, 2018 •
edited
Loading

joyeecheung commented Jun 27, 2018 •
edited
Loading

joyeecheung commented Jul 5, 2018 •
edited
Loading

joyeecheung Jun 11, 2019 •
edited
Loading