Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs.stat and fs.statSync may not return identical struct #12419

Closed
DoumanAsh opened this issue Apr 15, 2017 · 12 comments
Closed

fs.stat and fs.statSync may not return identical struct #12419

DoumanAsh opened this issue Apr 15, 2017 · 12 comments
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@DoumanAsh
Copy link

  • Version: 7.9.0
  • Platform: Windows 10 (64 bit)
  • Subsystem: fs

Simple code example is here: https://gist.github.com/DoumanAsh/a70b49b3e05aa4c4273dd6917cce13e0
Occasionally found that Stats.dev differs for sync and async versions.
I'm not really sure how it is possible considering that both should use the same OS API...
All other fields are identical

Sync stats output:

Stats {
  dev: 3163614271,
  mode: 16822,
  nlink: 1,
  uid: 0,
  gid: 0,
  rdev: 0,
  blksize: undefined,
  ino: 5066549581028165,
  size: 0,
  blocks: undefined,
  atime: 2017-04-15T06:12:53.786Z,
  mtime: 2017-04-15T06:12:53.786Z,
  ctime: 2017-04-15T06:12:53.786Z,
  birthtime: 2017-04-15T06:03:09.505Z }

Async stat output

Stats {
  dev: -1131353025,
  mode: 16822,
  nlink: 1,
  uid: 0,
  gid: 0,
  rdev: 0,
  blksize: undefined,
  ino: 5066549581028165,
  size: 0,
  blocks: undefined,
  atime: 2017-04-15T06:12:53.786Z,
  mtime: 2017-04-15T06:12:53.786Z,
  ctime: 2017-04-15T06:12:53.786Z,
  birthtime: 2017-04-15T06:03:09.505Z }
@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. labels Apr 15, 2017
@bnoordhuis
Copy link
Member

.dev is derived from VolumeSerialNumber, which is a 32 bits unsigned integer. It should fit comfortably in a 64 bits double and it should not (and should not be possible for it to) be less than zero.

Interestingly, 3163614271 and -1131353025 are the same 32 bits bit pattern so this is likely some integer->double conversion bug but it's not immediately obvious to me where it happens. Both the code in libuv and src/node_file.cc look okay at first glance.

@DoumanAsh
Copy link
Author

@bnoordhuis i didn't go as far as looking into libuv code, but at least i didn't spot anything in node code itself...

Sadly i don't have possibility to test it on any unix like system, so cannot say if it is windows specific bug or not.

@refack
Copy link
Contributor

refack commented Apr 15, 2017

Looking into it with windows glasses on

@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2017

@DoumanAsh Did you try with previous v7.x versions and/or other major versions?

@DoumanAsh
Copy link
Author

DoumanAsh commented Apr 15, 2017

@mscdex I remember having this issue in past on some older 7.x version, but cannot say for sure right now.

UPD:
Before v7.7 i get negative integer in both cases
After v7.7 we have current behavior

@seishun
Copy link
Contributor

seishun commented Apr 15, 2017

How did you create the file?

@DoumanAsh
Copy link
Author

Just gvim <file>

@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2017

The issue is that some optimizations were made to the fs.*statSync() methods awhile back that bring uniformity to how the stat values are represented (they're all doubles now, whereas previously it was all a mixture of 3 different types -- double, signed int, unsigned int). The changes to the fs.*stat() methods were included in a follow-up PR which was marked as semver-major by me because of other unrelated changes in that PR.

So either #11665 has to be backported, or only the fs.*stat()-related changes have to be backported. Also just to note, the positive values should actually be more correct more often than not now (due to larger unsigned value range in double vs signed integer).

@mscdex mscdex added v6.x and removed v6.x windows Issues and PRs related to the Windows platform. labels Apr 15, 2017
@mscdex mscdex mentioned this issue Apr 24, 2017
3 tasks
@Trott
Copy link
Member

Trott commented Aug 6, 2017

This is fixed in 8.x and the only affected currently-supported release line is 6.x? [EDIT: To be clear, that's a question, not a statement.]

@mscdex
Copy link
Contributor

mscdex commented Aug 6, 2017

@Trott yes, but I'm leaning towards just counting both PRs as semver-major (currently one is and the other isn't), which would mean no backporting at all. As far as the changes themselves go, changing the values in v6.x would be viewed as semver-major anyway, in case someone is relying on the signed-ness of the value.

@targos targos added v7.x and removed v7.x labels Oct 27, 2017
@targos targos removed the v7.x label Nov 5, 2017
@gireeshpunathil
Copy link
Member

@mscdex - this looks like a close candidate with no pending actions, right?

@gireeshpunathil
Copy link
Member

inactive, closing. please re-open if it is still outstanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

9 participants