fs: expose Stats times as Numbers by refack · Pull Request #13173 · nodejs/node

refack · 2017-05-23T16:17:45Z

Expose Stats times as Numbers
also set Date cache fields in constructor

Fixes: #8276
Fixes: npm/npm#16734
Ref: #12607
Ref: #12818

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

Affected core subsystem(s)

fs

refack · 2017-05-23T16:18:35Z

/cc @mscdex @jasnell @addaleax @TimothyGu @sciolist

refack · 2017-05-23T16:19:26Z

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

mscdex · 2017-05-23T16:38:31Z

test/parallel/test-fs-stat.js

nit: I think just parsed or similar would be a better variable name.

mscdex · 2017-05-23T16:38:54Z

test/parallel/test-fs-stat.js

Single quotes should be used for string literals.

These tests are failing because they're using "atime_msec" rather than "atim_msec", btw.

Ack. (actual lint)

mscdex · 2017-05-23T16:42:16Z

lib/fs.js

Also, can we change these names to add the missing 'e' (e.g. atim_msec -> atime_msec)?

I feel like snake_case is pretty uncommon in node, so should maybe be changed.. but mtimeMsec is kinda ugly, so I'm not going to suggest it.

I agree and tend to favor camel-case for js stuff, but I'm not sure about it in this case. I guess it's because the second part is an abbreviation. mtimeMs looks a little better though.

Just so you know, this is/was named this way because it derives from the POSIX stat struct that has existed with these names for a long time; I think it’s okay to keep the underscore here.

isn't posix just atim, mtim and ctim? but sure, timespec has tv_nsec etc.. and it doesn't matter.

I guess, yeah. If you want to go with mtimeMs, I’m not going to stand in your way or anything :)

@refack can decide, i'm good with whatever he prefers.

We've had the posix/non-posix discussion before for another feature in core, although I think that PR ended up dying because we couldn't decide ;-)

Went with camelCase Ms suffix.

addaleax · 2017-05-23T16:45:03Z

LGTM once @mscdex’s comments have been addressed. Also, CI failure (I assume that would fail with a local make test as well):

  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'undefined' === 'number'
    at /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-fs-stat.js:105:10
    at /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:517:15
    at FSReqWrap.oncomplete (fs.js:153:5)

sciolist · 2017-05-23T17:18:13Z

lib/fs.js

Should these not get this.birthtim_msec etc instead of the Date values?

Yep, they should be birthtime_msec, etc.

~~atime_msec: this._atime || atime_msec: this.atime would be better?`~~
Ack

Should just be, atime_msec: this.atime_msec,, I think? this is in the toJSON function, so I figure it should just expose that value.

refack · 2017-05-23T21:07:34Z

Also, CI failure (I assume that would fail with a local make test as well):

@addaleax Sorry I didn't label this in progress I wanted quick feedback and my local compile+test cycle is sooooo long.

New CI: https://ci.nodejs.org/job/node-test-commit/10100/

refack · 2017-05-23T21:10:00Z

doc/api/fs.md

I'll fix this after the CI passes

refack · 2017-05-23T21:10:18Z

doc/api/fs.md

cjihrig · 2017-05-23T22:07:53Z

test/parallel/test-fs-stat.js

Really tiny nit, but could you pick a different name for this variable.

Since the name I came up with is numerFields I decided to add all the number fields, so 👍

cjihrig · 2017-05-23T22:08:16Z

test/parallel/test-fs-stat.js

"should a" -> "should be a"

Ack.
But we both missed the bug: typeof s.birthtimeMs was not parameterized 🤦‍♂️

refack · 2017-05-23T22:28:35Z

New CI: https://ci.nodejs.org/job/node-test-commit/10102/

mscdex · 2017-05-23T22:53:53Z

doc/api/fs.md

These values don't agree with the Date-based fields shown below here.

Ack.
But that's a very nice nit 😄

mscdex · 2017-05-23T22:54:47Z

doc/api/fs.md

nit: s/instances of/instances of the/

Ack.
(That was there before, I just had to rewrap because of **Note:**)

mscdex · 2017-05-23T22:55:42Z

doc/api/fs.md

Perhaps this note about using .getTime() on the Date-based fields should be changed to suggest using the .*Ms fields instead.

Or maybe this whole paragraph should be rewritten to make note of the .*Ms fields first and then simply say that the non--Ms fields are simply Dates that use the .*Ms values? Hopefully that would simplify the wording a bit?

Working on it.

But Dates are easier to mutate, hence the bug.

mscdex · 2017-05-23T22:56:51Z

doc/api/fs.md

mscdex · 2017-05-23T22:57:55Z

doc/api/fs.md

s/times/times in milliseconds/.

mscdex · 2017-05-23T23:00:51Z

lib/fs.js

This isn't right. We should have tests to cover these to make sure the fields are what they should be.

Removed this low quality optimization all together.

refack · 2017-05-23T23:13:53Z

@mscdex and others. I've found a design bug:
After first getting any of the Dates the old value gets cached, so mutating it will not be reflected in the corresponding *Ms field.
I can handle assignment to the Dates in the setter:

    set(value) {
      this.atimeMs = Number(value);
      return this._atime = value;
    }

but mutations like s.mtime.setDate(newValue) are opaque (unless we proxy the date).
Also if we keep the cached Dates we get:

const old = s.mtime;
s.mtimeMs = newValue;
s.mtimeMs !== s.mtime;
s.mtime === old;

mscdex · 2017-05-23T23:18:20Z

I would just make note of it in the documentation that they are not linked together and are merely separate values that are initialized using the same set of data. Making the *Ms fields getters/setters probably introduces a performance hit, which wouldn't be worth it IMHO.

refack · 2017-05-23T23:32:52Z

I would just make note of it in the documentation that they are not linked together and are merely separate values that initialized using the same set of data. Making the *Ms fields getters/setters probably introduces a performance hit, which wouldn't be worth it IMHO.

Just realized that we don't accept a Stats as an argument, so it's less severe than I thought. Agreed that documentation is enough.

refack · 2017-05-24T02:13:09Z

New CI: https://ci.nodejs.org/job/node-test-commit/10110/

mscdex · 2017-05-24T02:58:39Z

lib/fs.js

I don't think this is a good idea since we don't update _ctime when ctimeMs is changed by end users and as I mentioned earlier, setting up a getter and setter for each number field to be able to do bidirectional updating is going to add noticeable overhead.

So you suggest break the connection completely, instead of this one way idiosyncrasy?
... thinking ...
Ok. Makes sense. (also congruent with the Note:)

Yes, that is what I'm suggesting.

addaleax · 2017-05-24T11:29:07Z

lib/fs.js

I’m pretty sure the linter complains about this; also, prefer the value() { return this.toJSON(); } shorthand

It's doesn't, but Ack.
I wanted to ask about the general approach of adding a [util.inspect.custom], is it Ok?

I wanted to ask about the general approach of adding a [util.inspect.custom], is it Ok?

Yes, I think that makes sense.

P.S. shorthand form lexical binds this so it should be linted for.

refack · 2017-06-01T14:57:40Z

~~"Dry run" CI: https://ci.nodejs.org/job/node-test-commit/10275/~~ wrong PR

refack · 2017-06-01T14:59:25Z

"Dry run" CI:https://ci.nodejs.org/job/node-test-commit/10276/

refack · 2017-06-01T22:06:55Z

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

refack · 2017-06-01T22:39:06Z

Linter: https://ci.nodejs.org/job/node-test-linter/9527/

refack · 2017-06-05T21:38:17Z

ping @mscdex @sciolist @thefourtheye @cjihrig @addaleax
Is there interest in moving forward with this (now completely semver-minor)?

mscdex · 2017-06-05T21:44:14Z

LGTM if CI is ok: https://ci.nodejs.org/job/node-test-pull-request/8494/

PR-URL: nodejs#13173 Fixes: nodejs#8276 Refs: nodejs#12607 Refs: nodejs#12818 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Brian White <mscdex@mscdex.net>

refack · 2017-06-07T20:17:18Z

Landed in 47b9772

refack · 2017-06-07T20:18:59Z

Extra sanity run on master:https://ci.nodejs.org/job/node-test-commit-linuxone/6462/

PR-URL: #13173 Fixes: #8276 Refs: #12607 Refs: #12818 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Brian White <mscdex@mscdex.net>

* **Async Hooks** * When one `Promise` leads to the creation of a new `Promise`, the parent `Promise` will be identified as the trigger [[`135f4e6643`](nodejs@135f4e6643)] [nodejs#13367](nodejs#13367). * **Dependencies** * libuv has been updated to 1.12.0 [[`968596ec77`](nodejs@968596ec77)] [nodejs#13306](nodejs#13306). * npm has been updated to 5.0.3 [[`ffa7debd7a`](nodejs@ffa7debd7a)] [nodejs#13487](nodejs#13487). * **File system** * The `fs.exists()` function now works correctly with `util.promisify()` [[`6e0eccd7a1`](nodejs@6e0eccd7a1)] [nodejs#13316](nodejs#13316). * fs.Stats times are now also available as numbers [[`c756efb25a`](nodejs@c756efb25a)] [nodejs#13173](nodejs#13173). * **Inspector** * It is now possible to bind to a random port using `--inspect=0` [[`cc6ec2fb27`](nodejs@cc6ec2fb27)] [nodejs#5025](nodejs#5025). * **Zlib** * A regression in the Zlib module that made it impossible to properly subclasses `zlib.Deflate` and other Zlib classes has been fixed. [[`6aeb555cc4`](nodejs@6aeb555cc4)] [nodejs#13374](nodejs#13374).

* **Async Hooks** * When one `Promise` leads to the creation of a new `Promise`, the parent `Promise` will be identified as the trigger [[`135f4e6643`](135f4e6643)] [#13367](#13367). * **Dependencies** * libuv has been updated to 1.12.0 [[`968596ec77`](968596ec77)] [#13306](#13306). * npm has been updated to 5.0.3 [[`ffa7debd7a`](ffa7debd7a)] [#13487](#13487). * **File system** * The `fs.exists()` function now works correctly with `util.promisify()` [[`6e0eccd7a1`](6e0eccd7a1)] [#13316](#13316). * fs.Stats times are now also available as numbers [[`c756efb25a`](c756efb25a)] [#13173](#13173). * **Inspector** * It is now possible to bind to a random port using `--inspect=0` [[`cc6ec2fb27`](cc6ec2fb27)] [#5025](#5025). * **Zlib** * A regression in the Zlib module that made it impossible to properly subclasses `zlib.Deflate` and other Zlib classes has been fixed. [[`6aeb555cc4`](6aeb555cc4)] [#13374](#13374).

MylesBorins · 2017-08-14T18:18:49Z

@nodejs/lts should we backport this to v6.x in a future minor?

sam-github · 2017-09-19T19:30:50Z

This is a follow-on to #12818, which is semver-major, so unbackportable.

But, this PR seems to try to make things more "backwards" compatible, right, by adding accessor properties? I think doing that kind of thing has itself been considered semver-major in the past.

Or perhaps I'm a bit lost in all the related issues, and also don't have time to read the epic commentary on this PR.

@refack what is the case for this being safe to backport? what are its dependent PRs, if any?

sciolist · 2017-09-20T11:21:21Z

It's not based on #12818, there just used to be a reference to it in some documentation. #12818 was reverted as it caused issues that were likely not fixable.

This PR indirectly depends on #13281 and #12607. (It will work without them, just won't be very useful.)

What it does is expose a few extra numbers on the Stats object, I don't think there's much reason to believe it's unsafe.

refack · 2017-09-20T11:26:48Z

What @sciolist wrote.
It's an independant minor, but needs #13281 and #12607 (both requested for backporting) for maximum usability.

I'll make a PR with the 3.

sam-github · 2017-09-20T17:50:47Z

@nodejs/lts Adding fields to a object returned by node (stats) could confuse some existing code if it was very poorly written, but it is good to keep API continuity between 6.x and later. I think I'm +1. What do you all think?

refack added fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version. labels May 23, 2017

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label May 23, 2017

mscdex reviewed May 23, 2017

View reviewed changes

sciolist reviewed May 23, 2017

View reviewed changes

refack self-assigned this May 23, 2017

refack commented May 23, 2017

View reviewed changes

doc/api/fs.md Outdated

Copy link

Contributor Author

refack May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this after the CI passes

refack commented May 23, 2017

View reviewed changes

doc/api/fs.md Outdated

Copy link

Contributor Author

refack May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

cjihrig reviewed May 23, 2017

View reviewed changes

mscdex reviewed May 23, 2017

View reviewed changes

refack force-pushed the improve-12818-public branch from 573025e to c1210b4 Compare May 24, 2017 02:10

mscdex reviewed May 24, 2017

View reviewed changes

addaleax reviewed May 24, 2017

View reviewed changes

refack force-pushed the improve-12818-public branch 2 times, most recently from d8525d7 to cedc47d Compare June 7, 2017 20:07

fs: expose Stats times as Numbers

47b9772

PR-URL: nodejs#13173 Fixes: nodejs#8276 Refs: nodejs#12607 Refs: nodejs#12818 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Brian White <mscdex@mscdex.net>

refack force-pushed the improve-12818-public branch from cedc47d to 47b9772 Compare June 7, 2017 20:16

refack merged commit 47b9772 into nodejs:master Jun 7, 2017

refack deleted the improve-12818-public branch June 7, 2017 20:27

refack added the notable-change PRs with changes that should be highlighted in changelogs. label Jun 7, 2017

jasnell pushed a commit that referenced this pull request Jun 7, 2017

fs: expose Stats times as Numbers

c756efb

PR-URL: #13173 Fixes: #8276 Refs: #12607 Refs: #12818 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Brian White <mscdex@mscdex.net>

refack removed their assignment Jun 12, 2017

not-an-aardvark mentioned this pull request Jun 13, 2017

v2.8.28's .tgz contains corrupt timestamps, causing errors from zip command and others mishoo/UglifyJS#2065

Closed

refack mentioned this pull request Jun 15, 2017

doc: fix api docs style #13700

Closed

2 tasks

refack mentioned this pull request Jul 10, 2017

fs.utimes truncate date #13520

Closed

refack mentioned this pull request Aug 14, 2017

src,fs: calculate times as unsigned long #13281

Merged

3 tasks

Uh oh!

Conversation

refack commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented May 23, 2017

Uh oh!

refack commented May 23, 2017

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!

mscdex May 23, 2017 • 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!

addaleax commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex May 23, 2017 • 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!

refack May 23, 2017 • 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!

refack commented May 23, 2017

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!

refack commented May 23, 2017

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

refack commented May 23, 2017 •

edited

Loading

mscdex May 23, 2017 •

edited

Loading

mscdex May 23, 2017 •

edited

Loading

refack May 23, 2017 •

edited

Loading

mscdex May 23, 2017 •

edited

Loading

mscdex May 23, 2017 •

edited

Loading

mscdex commented May 23, 2017 •

edited

Loading