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

utimes doesn't seem to support subseconds times #22070

Closed
arcanis opened this issue Aug 1, 2018 · 6 comments
Closed

utimes doesn't seem to support subseconds times #22070

arcanis opened this issue Aug 1, 2018 · 6 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.

Comments

@arcanis
Copy link
Contributor

arcanis commented Aug 1, 2018

  • Version: 10.7.0 / 8.11.3
  • Platform: OSX & Linux
  • Subsystem: fs

Not sure if it's an implementation issue or a documentation issue, but the following is unexpected:

> var d = new Date(1234567891234);

> fs.utimesSync('./test', d, d);
> fs.statSync('./test').mtimeMs
1234567891000

> fs.futimesSync(fs.openSync('./test', 'w'), d, d);
> fs.statSync('./test').mtimeMs
1234567891234

As you can see, the precision is lost (my systems support subsecond precisions - touch manages to change them, and both the stat utility and fs.stat can read them without issues).

The Node documentation also doesn't mention that utimes has a different behavior from futimes (it actually suggest they share the exact same behavior, since futimes redirect to utimes), except possibly in this archived post.

Additionally, the function name (utimes) seem to match the behavior of 2 utime instead of 3 utimes, which seems counterintuitive. The libuv implementation is called uv_fs_utime , which at least hints at the difference of behavior (except that they use uv_fs_futime instead of uv_fs_futimes ..).

@Fishrock123
Copy link
Member

Fishrock123 commented Aug 2, 2018

@arcanis Are you sure this affects Linux? What version of MacOS did you test? If you tested one that has APFS, are you sure you aren't running into this MacOS APFS Driver bug? - https://openradar.appspot.com/radar?id=4978184932032512

arcanis added a commit to yarnpkg/yarn that referenced this issue Aug 3, 2018
* Revert "fix(util/fs): use file content instead of mtime to determine equality (#6010)"

This reverts commit c53d039.

* Bumps the cache version

* Forces the mtime to be based on the time the archives got fetched

* Fixes subseconds time assignment (nodejs/node#22070)

* Adds regression tests for #5723

* Fixes tests

* Fixes test on architectures that don't support subsecond precision
arcanis added a commit to yarnpkg/yarn that referenced this issue Aug 3, 2018
* Revert "fix(util/fs): use file content instead of mtime to determine equality (#6010)"

This reverts commit c53d039.

* Bumps the cache version

* Forces the mtime to be based on the time the archives got fetched

* Fixes subseconds time assignment (nodejs/node#22070)

* Adds regression tests for #5723

* Fixes tests

* Fixes test on architectures that don't support subsecond precision
@Fishrock123 Fishrock123 added the fs Issues and PRs related to the fs subsystem / file system. label Aug 3, 2018
@arcanis
Copy link
Contributor Author

arcanis commented Aug 4, 2018

@Fishrock123 I'll double check, but I'm pretty sure. Can't say the exact MacOS release, but I don't believe I was affected by the bug you mentioned (simply because touch, stat, and futimes are all working correctly. Additionnally, your link references the full date part being set to a specific value, while in my case only the subsecond part is being zero'd).

Tracking down the issue, it seems to be that the utimes implementation is simply using utime (instead of the real utimes). Weirdly the uv function has the correct name (utime, no s), so I'm not entirely sure where the boggus renaming happens.

node/deps/uv/src/unix/fs.c

Lines 700 to 705 in 537a4ba

static ssize_t uv__fs_utime(uv_fs_t* req) {
struct utimbuf buf;
buf.actime = req->atime;
buf.modtime = req->mtime;
return utime(req->path, &buf); /* TODO use utimes() where available */
}

The utime() system call allows specification of timestamps with a resolution of 1 second. (link)

@Fishrock123
Copy link
Member

Yeah, I also saw that while debugging my problem.

I'll try to get a libuv pr up this week.

@Fishrock123 Fishrock123 added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Aug 7, 2018
Fishrock123 added a commit to Fishrock123/libuv that referenced this issue Aug 7, 2018
This should improve uv_fs_utime resolution and reliability, as utime(2)'s precision is left more to the implementing platform than the newer but well supported alternatives.

Related to nodejs/node#22070
Fishrock123 added a commit to Fishrock123/libuv that referenced this issue Aug 7, 2018
This should improve uv_fs_utime resolution and reliability, as
utime(2)'s precision is left more to the implementing platform than the
newer but well supported alternatives.

Related to nodejs/node#22070
Fishrock123 added a commit to Fishrock123/libuv that referenced this issue Aug 24, 2018
This should improve uv_fs_utime resolution and reliability, as
utime(2)'s precision is left more to the implementing platform than the
newer but well supported alternatives.

Related to nodejs/node#22070
PR-URL: libuv#1940
santigimeno pushed a commit to libuv/libuv that referenced this issue Sep 3, 2018
This should improve uv_fs_utime resolution and reliability, as
utime(2)'s precision is left more to the implementing platform than the
newer but well supported alternatives.

Related to nodejs/node#22070
PR-URL: #1940
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@Fishrock123
Copy link
Member

Libuv PR has been merged: libuv/libuv#1940 - should fix it but never got to actually trying if it solves this. 😅

@cjihrig
Copy link
Contributor

cjihrig commented Sep 30, 2018

@arcanis can you confirm if your problem is solved with the latest on master (including libuv 1.23.1)?

@Fishrock123
Copy link
Member

I can confirm this is fixed. \o/

Fixed in 2790db5#diff-e8b03d17fba605de23431c53172682a5 / #22997 / Node 11.0.0

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. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

No branches or pull requests

3 participants