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

utime behaviour different from MRI #4710

Closed
gioele opened this Issue Jul 8, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@gioele

gioele commented Jul 8, 2017

The following code works as expected in MRI, but fails in JRuby 9.1.9.0.

F = '/tmp/test'
File.open(F, 'w') {} # "touch" the file
mtime = File.mtime(F)
File.utime(Time.now, 0, F)
File.mtime(F).to_i == 0 or raise "mtime should be 0"

File.utime(Time.now, mtime, F)
File.mtime(F).to_i == mtime.to_i or raise "File.mtime should be == mtime"

Found while testing the filepath gem via Travis-CI. The log is available at https://travis-ci.org/gioele/filepath/jobs/251546194

@BanzaiMan

This comment has been minimized.

Show comment
Hide comment
@BanzaiMan

BanzaiMan Jul 8, 2017

Member

IIRC, JVM does not provide the same granularity of time, so this can't be fixed.

Member

BanzaiMan commented Jul 8, 2017

IIRC, JVM does not provide the same granularity of time, so this can't be fixed.

@gioele

This comment has been minimized.

Show comment
Hide comment
@gioele

gioele Jul 8, 2017

I was expecting a difference between MRI and JVM, but it seems strange (unexpected, at least) that inside the JVM File#mtime has a different time granularity than File#utime.

gioele commented Jul 8, 2017

I was expecting a difference between MRI and JVM, but it seems strange (unexpected, at least) that inside the JVM File#mtime has a different time granularity than File#utime.

@alexis779

This comment has been minimized.

Show comment
Hide comment
@alexis779

alexis779 Sep 16, 2017

Contributor

JRuby implements File.utime with utimes native call and timeval data structure which precision is the microsecond and not the nanosecond.

We could update JRuby utime to call libc utimenstat instead of utimes. Would this still be POSIX? It would be using timespec struct instead to be on par with Ruby.

We would need to create the struct and add the method in the native interface in jnr-posix lib?

Test Failure

It seems the cast to integer in your snippet does not make the test fail.
Compare with the test from your gem currently failing with jruby:

Failures:

  1) Filepath Filepath::MetadataChanges#chtime change mtime
     Failure/Error: ph.mtime.should eq(orig_mtime)
     
       expected: 2017-09-16 11:08:45.660759724 -0700
            got: 2017-09-16 11:08:45.660759000 -0700
     
       (compared using ==)
     
       Diff:
       
     # ./spec/filepath_spec.rb:589:in `block in (root)'

Finished in 1.61 seconds (files took 1.87 seconds to load)
227 examples, 1 failure

Failed examples:

rspec ./spec/filepath_spec.rb:581 # Filepath Filepath::MetadataChanges#chtime change mtime

Links

Contributor

alexis779 commented Sep 16, 2017

JRuby implements File.utime with utimes native call and timeval data structure which precision is the microsecond and not the nanosecond.

We could update JRuby utime to call libc utimenstat instead of utimes. Would this still be POSIX? It would be using timespec struct instead to be on par with Ruby.

We would need to create the struct and add the method in the native interface in jnr-posix lib?

Test Failure

It seems the cast to integer in your snippet does not make the test fail.
Compare with the test from your gem currently failing with jruby:

Failures:

  1) Filepath Filepath::MetadataChanges#chtime change mtime
     Failure/Error: ph.mtime.should eq(orig_mtime)
     
       expected: 2017-09-16 11:08:45.660759724 -0700
            got: 2017-09-16 11:08:45.660759000 -0700
     
       (compared using ==)
     
       Diff:
       
     # ./spec/filepath_spec.rb:589:in `block in (root)'

Finished in 1.61 seconds (files took 1.87 seconds to load)
227 examples, 1 failure

Failed examples:

rspec ./spec/filepath_spec.rb:581 # Filepath Filepath::MetadataChanges#chtime change mtime

Links

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Dec 14, 2017

Member

the original report seems to be resolved by (latest) 9.1.15.0 :

jruby-9.1.15.0 :001 > F = '/tmp/test'
 => "/tmp/test" 
jruby-9.1.15.0 :002 > File.open(F, 'w') {} # "touch" the file
 => nil 
jruby-9.1.15.0 :003 > mtime = File.mtime(F)
 => 2017-12-14 12:43:22 +0100 
jruby-9.1.15.0 :004 > File.utime(Time.now, 0, F)
 => 1 
jruby-9.1.15.0 :005 > File.mtime(F).to_i == 0 or raise "mtime should be 0"
 => true 
jruby-9.1.15.0 :006 > 
jruby-9.1.15.0 :007 >   File.utime(Time.now, mtime, F)
 => 1 
jruby-9.1.15.0 :008 > File.mtime(F).to_i == mtime.to_i or raise "File.mtime should be == mtime"
 => true 
jruby-9.1.15.0 :009 > JRUBY_VERSION
 => "9.1.15.0" 

... for the second issue there's already a separate issue, right? any objections on closing this one?

Member

kares commented Dec 14, 2017

the original report seems to be resolved by (latest) 9.1.15.0 :

jruby-9.1.15.0 :001 > F = '/tmp/test'
 => "/tmp/test" 
jruby-9.1.15.0 :002 > File.open(F, 'w') {} # "touch" the file
 => nil 
jruby-9.1.15.0 :003 > mtime = File.mtime(F)
 => 2017-12-14 12:43:22 +0100 
jruby-9.1.15.0 :004 > File.utime(Time.now, 0, F)
 => 1 
jruby-9.1.15.0 :005 > File.mtime(F).to_i == 0 or raise "mtime should be 0"
 => true 
jruby-9.1.15.0 :006 > 
jruby-9.1.15.0 :007 >   File.utime(Time.now, mtime, F)
 => 1 
jruby-9.1.15.0 :008 > File.mtime(F).to_i == mtime.to_i or raise "File.mtime should be == mtime"
 => true 
jruby-9.1.15.0 :009 > JRUBY_VERSION
 => "9.1.15.0" 

... for the second issue there's already a separate issue, right? any objections on closing this one?

@kares kares closed this Dec 16, 2017

@kares kares added this to the Invalid or Duplicate milestone Dec 16, 2017

@alexis779

This comment has been minimized.

Show comment
Hide comment
@alexis779

alexis779 Dec 17, 2017

Contributor

I am not sure this is fixed. Can you try again, line 8 without the cast to int:

File.mtime(F) == mtime or raise "File.mtime should be == mtime"
Contributor

alexis779 commented Dec 17, 2017

I am not sure this is fixed. Can you try again, line 8 without the cast to int:

File.mtime(F) == mtime or raise "File.mtime should be == mtime"
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Dec 18, 2017

Member

@alexis779 you're right, noticed the jnr-posix links here - but was weird that the example passed

someone from @jruby/core should take a look at your work, for reference here's the failing script :

F = '/tmp/test'
File.open(F, 'w') {} # "touch" the file
mtime = File.mtime(F)
File.utime(Time.now, 0, F)
File.mtime(F).to_i == 0 or raise "mtime should be 0"

File.utime(Time.now, mtime, F)
File.mtime(F) == mtime or raise "File.mtime should be == mtime"
Member

kares commented Dec 18, 2017

@alexis779 you're right, noticed the jnr-posix links here - but was weird that the example passed

someone from @jruby/core should take a look at your work, for reference here's the failing script :

F = '/tmp/test'
File.open(F, 'w') {} # "touch" the file
mtime = File.mtime(F)
File.utime(Time.now, 0, F)
File.mtime(F).to_i == 0 or raise "mtime should be 0"

File.utime(Time.now, mtime, F)
File.mtime(F) == mtime or raise "File.mtime should be == mtime"

@kares kares reopened this Dec 18, 2017

@kares kares removed this from the Invalid or Duplicate milestone Dec 18, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 19, 2017

Member

Not all platforms provide this granularity of filesystem times, which is the main reason we tend to default to what the JVM provides, usually millisecond granularity at best.

However we have made improvements over time to support the higher-granularity filesystem times when we can call to the appropriate native structures. For example, see 41f2bb6 which added nanosecond granularity (where supported by the platform and by jnr-posix) for atime, ctime, and mtime.

@gioele Your logic for using futimens seems just fine, but we should put it in the jnr-posix library. Can you have a look at https://github.com/jnr/jnr-posix and see if you can move your FFI changes there?

Member

headius commented Dec 19, 2017

Not all platforms provide this granularity of filesystem times, which is the main reason we tend to default to what the JVM provides, usually millisecond granularity at best.

However we have made improvements over time to support the higher-granularity filesystem times when we can call to the appropriate native structures. For example, see 41f2bb6 which added nanosecond granularity (where supported by the platform and by jnr-posix) for atime, ctime, and mtime.

@gioele Your logic for using futimens seems just fine, but we should put it in the jnr-posix library. Can you have a look at https://github.com/jnr/jnr-posix and see if you can move your FFI changes there?

@alexis779

This comment has been minimized.

Show comment
Hide comment
@alexis779

alexis779 Dec 19, 2017

Contributor

FFI changes are already in a new jnr-posix PR: jnr/jnr-posix#107

In #4795 we are just calling new futimens method from POSIX Java interface.

You are right if the platform does not support futimens, updating the modification time via jruby will not work anymore because futimens is not implemented in the JavaPOSIX interface.

Contributor

alexis779 commented Dec 19, 2017

FFI changes are already in a new jnr-posix PR: jnr/jnr-posix#107

In #4795 we are just calling new futimens method from POSIX Java interface.

You are right if the platform does not support futimens, updating the modification time via jruby will not work anymore because futimens is not implemented in the JavaPOSIX interface.

alexis779 added a commit to alexis779/jruby that referenced this issue Jan 17, 2018

alexis779 added a commit to alexis779/jruby that referenced this issue Jan 24, 2018

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 24, 2018

Member

@alexis779 Excellent thank you! I'll see about pulling it in and utilizing it in JRuby.

Member

headius commented Jan 24, 2018

@alexis779 Excellent thank you! I'll see about pulling it in and utilizing it in JRuby.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 25, 2018

Member

@alexis779 I see some activity on your JRuby commit for jnr-posix. I will push the release with futimes and update JRuby to use it. From there, can you put together a PR for the JRuby part, once you've confirmed it's working?

Member

headius commented Jan 25, 2018

@alexis779 I see some activity on your JRuby commit for jnr-posix. I will push the release with futimes and update JRuby to use it. From there, can you put together a PR for the JRuby part, once you've confirmed it's working?

alexis779 added a commit to alexis779/jruby that referenced this issue Jan 27, 2018

@alexis779

This comment has been minimized.

Show comment
Hide comment
@alexis779

alexis779 Jan 27, 2018

Contributor

Thanks for merging jnr-posix PR. Updated #4795 jruby PR

Test

F = '/tmp/test'
mtime = File.mtime(F)

File.utime(Time.now, mtime, F)
File.mtime(F) == mtime or raise "File.mtime should be == mtime"

Reproduction case

$ rbenv local jruby-9.1.12.0
$ ~/.rbenv/shims/ruby -v
jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439 OpenJDK 64-Bit Server VM 25.144-b01 on 1.8.0_144-8u144-b01-1-b01 +jit [linux-x86_64]
$ ~/.rbenv/shims/ruby test/utime.rb 
RuntimeError: File.mtime should be == mtime
  <main> at test/utime.rb:5

Test fails. The nanosecond precision in Modify date was lost after the utimes call.

With the patch

$ stat test/test
Access: 2018-01-27 12:57:59.377652497 -0800
Modify: 2018-01-27 12:57:59.377652497 -0800
$ bin/ruby test/utime.rb
$ stat test/test
Access: 2018-01-27 12:58:21.225190000 -0800
Modify: 2018-01-27 12:57:59.377652497 -0800

Test passes. The nanosecond precision in Modify date was maintained after the utimensat call.

Windows

$ touch test/test
$ stat test/test
Access: 2018-01-27 20:50:27.868916300 +0000
Modify: 2018-01-27 20:50:27.868916300 +0000
$ bin/jruby -v
jruby 9.2.0.0-SNAPSHOT (2.4.1) 2018-01-27 87d7706 Java HotSpot(TM) 64-Bit Server VM 25.152-b16 on 1.8.0_152-b16 +jit [mswin32-x86_64]
$ bin/jruby test/utime.rb
$ stat test/test
Access: 2018-01-27 20:50:48.315000000 +0000
Modify: 2018-01-27 20:50:27.000000000 +0000

JRuby Limitations

  • Linux
    • Epoch timestamp precision of Time.now is microsecond.
  • Windows
    • Epoch timestamp precision is millisecond.
    • stat class only has second precision, we could get 100 ns precision updating stat implementation class ?
Contributor

alexis779 commented Jan 27, 2018

Thanks for merging jnr-posix PR. Updated #4795 jruby PR

Test

F = '/tmp/test'
mtime = File.mtime(F)

File.utime(Time.now, mtime, F)
File.mtime(F) == mtime or raise "File.mtime should be == mtime"

Reproduction case

$ rbenv local jruby-9.1.12.0
$ ~/.rbenv/shims/ruby -v
jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439 OpenJDK 64-Bit Server VM 25.144-b01 on 1.8.0_144-8u144-b01-1-b01 +jit [linux-x86_64]
$ ~/.rbenv/shims/ruby test/utime.rb 
RuntimeError: File.mtime should be == mtime
  <main> at test/utime.rb:5

Test fails. The nanosecond precision in Modify date was lost after the utimes call.

With the patch

$ stat test/test
Access: 2018-01-27 12:57:59.377652497 -0800
Modify: 2018-01-27 12:57:59.377652497 -0800
$ bin/ruby test/utime.rb
$ stat test/test
Access: 2018-01-27 12:58:21.225190000 -0800
Modify: 2018-01-27 12:57:59.377652497 -0800

Test passes. The nanosecond precision in Modify date was maintained after the utimensat call.

Windows

$ touch test/test
$ stat test/test
Access: 2018-01-27 20:50:27.868916300 +0000
Modify: 2018-01-27 20:50:27.868916300 +0000
$ bin/jruby -v
jruby 9.2.0.0-SNAPSHOT (2.4.1) 2018-01-27 87d7706 Java HotSpot(TM) 64-Bit Server VM 25.152-b16 on 1.8.0_152-b16 +jit [mswin32-x86_64]
$ bin/jruby test/utime.rb
$ stat test/test
Access: 2018-01-27 20:50:48.315000000 +0000
Modify: 2018-01-27 20:50:27.000000000 +0000

JRuby Limitations

  • Linux
    • Epoch timestamp precision of Time.now is microsecond.
  • Windows
    • Epoch timestamp precision is millisecond.
    • stat class only has second precision, we could get 100 ns precision updating stat implementation class ?

headius added a commit that referenced this issue Feb 13, 2018

Merge pull request #4795 from alexis779/master
[#4710] nanosecond precision in utime using libc futimens

headius added a commit that referenced this issue Feb 13, 2018

Merge pull request #4795 from alexis779/master
[#4710] nanosecond precision in utime using libc futimens
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 13, 2018

Member

I've merged #4795 to master and 9.1.16.0. Thank you for your help!

Member

headius commented Feb 13, 2018

I've merged #4795 to master and 9.1.16.0. Thank you for your help!

@headius headius closed this Feb 13, 2018

@headius headius added this to the JRuby 9.1.16.0 milestone Feb 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment