marschall / memoryfilesystem

An in memory implementation of a JSR-203 file system
284 stars 36 forks source link

Let's use JDK10 and use FileTime instead of FileTime::toMillis #101

Closed glhez closed 5 years ago

glhez commented 6 years ago

In some of my test, I was building a FileTime using this:

final Path path = ...; // memory FS Path
final Instant base = Instant.now();
final BasicFileAttributeView fav = Files.getFileAttributeView(path, BasicFileAttributeView.class);
fav.setTimes(base, base, base);
assertEquals(base.toString(), Files.lastModifiedTime(base).toString()); // fails

The assertEquals fails with message Expecting actual's toString() to return: <"2018-07-09T19:23:11.5957207Z"> but was: <2018-07-09T19:23:11.595Z> (I'm actually using assertj, so the message might differ from classic JUnit).

I found that, in MemoryEntryAttributes, the code was using long to store milliseconds instead of FileTime. I don't know the rationals behind this choice, but it loose micros/nano seconds information when available (which seems to be the case in Java 10). Also, in setTimes, there was a writeLock but I did never find a readLock() when accessing the properties, so I added it.

Finally, since I was using JDK 10, I introduced it to the build.

Build which fails for Spring reasons.

There are the same than when dealing with Mockito in Java 10: https://stackoverflow.com/questions/50634322/java-modules-accessibility-problems-for-mockito-2-19-0

marschall commented 6 years ago

Thank you. However this requires some thought from me:

  1. As we're mimicking the behaviour of of MacOS/Linux/Windows file systems we should probably make the resolution configurable (see table below). Your code is not guaranteed to work, especially on Java 9+ with nanosecond Instant.now(). We should be able to simulate this.
  2. Ideally getNow() is able to make use of nanosecond resolution on Java 9+.

| HFS+ | 1 s | | APFS | 1 ns | | Ext4 | 1 ns | | NTFS | 100 ns |

glhez commented 5 years ago

Hello, I don't know what your status about using FileTime (or not), however there were fix in use of locks (see https://github.com/marschall/memoryfilesystem/compare/master...glhez:master#diff-27fcdc9e12d2b0c06dcfdf71e7a9ed42R98).

Would you rather that 1) I create a separate commit/pr to fix the lock issue (you are using a writeLock but you do not protect the stuff being wrote against reads) 2) I create an issue for the Time ? 3) I close this PR

Regards, glhez

marschall commented 5 years ago

The master branch is on Java 8 now so we can use java.time types. I would prefer:

I can then create an issue where we can figure out how we correctly truncate the times depending on the platform.

glhez commented 5 years ago

Ok. I close this PR then.