From 6b95332e451fd701ccf199dadd1b42827ee158ce Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Tue, 24 Sep 2024 22:01:38 +0200 Subject: [PATCH] [7.4.0] Use millisecond precision when setting the last modified time on Unix. (#23740) Currently, we truncate to second precision. Instead, make use of utimensat(2), which can accommodate up to nanosecond precision, is part of POSIX, and is widely available (Linux, Darwin, OpenBSD, FreeBSD and NetBSD). PiperOrigin-RevId: 678185275 Change-Id: I307dbc75c8f594e6d70b43ee0bde2226b3015611 --- .../build/lib/unix/NativePosixFiles.java | 15 +++++---- .../build/lib/unix/UnixFileSystem.java | 8 +---- .../build/lib/vfs/JavaIoFileSystem.java | 7 ---- src/main/native/unix_jni.cc | 33 ++++++++----------- .../build/lib/vfs/FileSystemTest.java | 26 +++++++++++++++ .../build/lib/vfs/JavaIoFileSystemTest.java | 23 +------------ 6 files changed, 50 insertions(+), 62 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java b/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java index d29e63739331c1..3b28f056402b55 100644 --- a/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java +++ b/src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java @@ -128,16 +128,17 @@ public static native void symlink(String oldpath, String newpath) public static native ErrnoFileStatus errnoLstat(String path); /** - * Native wrapper around POSIX utime(2) syscall. + * Native wrapper around POSIX utimensat(2) syscall. * - * Note: negative file times are interpreted as unsigned time_t. + *

Note that, even though utimensat(2) supports up to nanosecond precision, this interface only + * allows millisecond precision, which is what Bazel uses internally. * - * @param path the file whose times to change. - * @param now if true, ignore actime/modtime parameters and use current time. - * @param modtime the file modification time in seconds since the UNIX epoch. - * @throws IOException if the utime() syscall failed. + * @param path the file whose modification time should be changed. + * @param now if true, ignore {@code epochMilli} and use the current time. + * @param epochMilli the file modification time in milliseconds since the UNIX epoch. + * @throws IOException if the operation failed. */ - public static native void utime(String path, boolean now, int modtime) throws IOException; + public static native void utimensat(String path, boolean now, long epochMilli) throws IOException; /** * Native wrapper around POSIX mkdir(2) syscall. diff --git a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java index c16c8164f39f01..e71f7e9f37a1f4 100644 --- a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java @@ -407,13 +407,7 @@ protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) th @Override public void setLastModifiedTime(PathFragment path, long newTime) throws IOException { - if (newTime == Path.NOW_SENTINEL_TIME) { - NativePosixFiles.utime(path.toString(), true, 0); - } else { - // newTime > MAX_INT => -ve unixTime - int unixTime = (int) (newTime / 1000); - NativePosixFiles.utime(path.toString(), false, unixTime); - } + NativePosixFiles.utimensat(path.toString(), newTime == Path.NOW_SENTINEL_TIME, newTime); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java index 71955e744c0342..cb74d3f22d2682 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/JavaIoFileSystem.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.vfs; -import com.google.common.annotations.VisibleForTesting; import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; @@ -63,12 +62,6 @@ public JavaIoFileSystem(DigestHashFunction hashFunction) { this.clock = new JavaClock(); } - @VisibleForTesting - JavaIoFileSystem(Clock clock, DigestHashFunction hashFunction) { - super(hashFunction); - this.clock = clock; - } - protected File getIoFile(PathFragment path) { return new File(path.toString()); } diff --git a/src/main/native/unix_jni.cc b/src/main/native/unix_jni.cc index acdbef5c83d597..d97ea7b5bd244d 100644 --- a/src/main/native/unix_jni.cc +++ b/src/main/native/unix_jni.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -30,6 +31,7 @@ #include #include #include +#include #include #include @@ -489,32 +491,25 @@ Java_com_google_devtools_build_lib_unix_NativePosixFiles_errnoLstat(JNIEnv *env, /* * Class: com.google.devtools.build.lib.unix.NativePosixFiles - * Method: utime - * Signature: (Ljava/lang/String;ZII)V + * Method: utimensat + * Signature: (Ljava/lang/String;ZJ)V * Throws: java.io.IOException */ extern "C" JNIEXPORT void JNICALL -Java_com_google_devtools_build_lib_unix_NativePosixFiles_utime(JNIEnv *env, - jclass clazz, - jstring path, - jboolean now, - jint modtime) { +Java_com_google_devtools_build_lib_unix_NativePosixFiles_utimensat( + JNIEnv *env, jclass clazz, jstring path, jboolean now, jlong millis) { const char *path_chars = GetStringLatin1Chars(env, path); -#ifdef __linux - struct timespec spec[2] = {{0, UTIME_OMIT}, {modtime, now ? UTIME_NOW : 0}}; + int64_t sec = millis / 1000; + int32_t nsec = (millis % 1000) * 1000000; + struct timespec spec[2] = { + // Do not set atime. + {0, UTIME_OMIT}, + // Set mtime to now if `now` is true, otherwise to the specified time. + {sec, now ? UTIME_NOW : nsec}, + }; if (::utimensat(AT_FDCWD, path_chars, spec, 0) == -1) { PostException(env, errno, path_chars); } -#else - struct utimbuf buf = { modtime, modtime }; - struct utimbuf *bufptr = now ? nullptr : &buf; - if (::utime(path_chars, bufptr) == -1) { - // EACCES ENOENT EMULTIHOP ELOOP EINTR - // ENOTDIR ENOLINK EPERM EROFS -> IOException - // EFAULT ENAMETOOLONG -> RuntimeException - PostException(env, errno, path_chars); - } -#endif ReleaseStringLatin1Chars(path_chars); } diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java index 382502351f2934..58e05a2b35571b 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java @@ -43,6 +43,7 @@ import java.nio.channels.WritableByteChannel; import java.nio.charset.StandardCharsets; import java.nio.file.FileAlreadyExistsException; +import java.time.Instant; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicBoolean; import java.util.regex.Matcher; @@ -1177,6 +1178,31 @@ public void testDeleteTreesBelowFailsGracefullyIfContentsGoMissing() throws Exce } // Test the date functions + + @Test + public void testSetLastModifiedTime() throws Exception { + Path file = absolutize("file"); + FileSystemUtils.createEmptyFile(file); + + file.setLastModifiedTime(1234567890L); + assertThat(file.getLastModifiedTime()).isEqualTo(1234567890L); + } + + @Test + public void testSetLastModifiedTimeWithSentinel() throws Exception { + Path file = absolutize("file"); + FileSystemUtils.createEmptyFile(file); + + // To avoid sleeping, first set the modification time to the past. + long pastTime = Instant.now().minusSeconds(1).toEpochMilli(); + file.setLastModifiedTime(pastTime); + + // Even if we get the system time before the setLastModifiedTime call, getLastModifiedTime may + // return a time which is slightly behind. Simply check that it's greater than the past time. + file.setLastModifiedTime(Path.NOW_SENTINEL_TIME); + assertThat(file.getLastModifiedTime()).isGreaterThan(pastTime); + } + @Test public void testCreateFileChangesTimeOfDirectory() throws Exception { storeReferenceTime(workingDir.getLastModifiedTime()); diff --git a/src/test/java/com/google/devtools/build/lib/vfs/JavaIoFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/vfs/JavaIoFileSystemTest.java index ab5b9d308f48a7..b8ca6f69d2d050 100644 --- a/src/test/java/com/google/devtools/build/lib/vfs/JavaIoFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/vfs/JavaIoFileSystemTest.java @@ -13,14 +13,11 @@ // limitations under the License. package com.google.devtools.build.lib.vfs; -import static com.google.common.truth.Truth.assertThat; - import com.google.common.collect.Iterables; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; -import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.TestUtils; import java.io.IOException; import java.nio.file.Files; @@ -42,12 +39,9 @@ */ public class JavaIoFileSystemTest extends SymlinkAwareFileSystemTest { - private ManualClock clock; - @Override public FileSystem getFreshFileSystem(DigestHashFunction digestHashFunction) { - clock = new ManualClock(); - return new JavaIoFileSystem(clock, digestHashFunction); + return new JavaIoFileSystem(digestHashFunction); } // Tests are inherited from the FileSystemTest @@ -58,21 +52,6 @@ public FileSystem getFreshFileSystem(DigestHashFunction digestHashFunction) { @Test public void testBadPermissionsThrowsExceptionOnStatIfFound() {} - @Test - public void testSetLastModifiedTime() throws Exception { - Path file = xEmptyDirectory.getChild("new-file"); - FileSystemUtils.createEmptyFile(file); - - file.setLastModifiedTime(1000L); - assertThat(file.getLastModifiedTime()).isEqualTo(1000L); - file.setLastModifiedTime(0L); - assertThat(file.getLastModifiedTime()).isEqualTo(0L); - - clock.advanceMillis(42000L); - file.setLastModifiedTime(-1L); - assertThat(file.getLastModifiedTime()).isEqualTo(42000L); - } - @Override protected boolean isHardLinked(Path a, Path b) throws IOException { return Files.readAttributes(