[8.0.0] Make DiskCacheLock a generic utility class for holding a file…

…system lock. (#24145) So that it can be reused by other features that require filesystem-level synchronization. PiperOrigin-RevId: 691496588 Change-Id: I7ab75b701f448fa50eedfe7090e124156d2cdf38
bazelbuild · Oct 30, 2024 · a98e1be · a98e1be
1 parent cd267a1
commit a98e1be
Show file tree

Hide file tree

Showing 11 changed files with 90 additions and 60 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/BUILD b/src/main/java/com/google/devtools/build/lib/remote/disk/BUILD
@@ -22,7 +22,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote/options",
         "//src/main/java/com/google/devtools/build/lib/remote/util",
         "//src/main/java/com/google/devtools/build/lib/server:idle_task",
-        "//src/main/java/com/google/devtools/build/lib/util:string",
+        "//src/main/java/com/google/devtools/build/lib/util:file_system_lock",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//third_party:flogger",
         "//third_party:guava",

diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollector.java
@@ -21,6 +21,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor;
 import com.google.devtools.build.lib.concurrent.ErrorClassifier;
+import com.google.devtools.build.lib.util.FileSystemLock;
 import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.IORuntimeException;
@@ -182,7 +183,7 @@ public CollectionPolicy getPolicy() {
   public CollectionStats run() throws IOException, InterruptedException {
     // Acquire an exclusive lock to prevent two Bazel processes from simultaneously running
     // garbage collection, which can waste resources and lead to incorrect results.
-    try (var lock = DiskCacheLock.getExclusive(root.getRelative("gc/lock"))) {
+    try (var lock = FileSystemLock.getExclusive(root.getRelative("gc/lock"))) {
       return runUnderLock();
     }
   }

diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD
@@ -42,6 +42,15 @@ java_library(
     deps = ["//third_party:guava"],
 )
 
+java_library(
+    name = "file_system_lock",
+    srcs = ["FileSystemLock.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//third_party:guava",
+    ],
+)
+
 java_library(
     name = "process",
     srcs = ["ProcessUtils.java"],

diff --git a/.../build/lib/remote/disk/DiskCacheLock.java → ...vtools/build/lib/util/FileSystemLock.java b/.../build/lib/remote/disk/DiskCacheLock.java → ...vtools/build/lib/util/FileSystemLock.java
@@ -11,7 +11,7 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-package com.google.devtools.build.lib.remote.disk;
+package com.google.devtools.build.lib.util;
 
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
@@ -23,39 +23,41 @@
 import java.nio.charset.Charset;
 import java.nio.file.StandardOpenOption;
 
-/** Manages shared or exclusive access to the disk cache by concurrent processes. */
-public final class DiskCacheLock implements AutoCloseable {
+/**
+ * Manages shared or exclusive access to the filesystem by concurrent processes through a lock file.
+ */
+public final class FileSystemLock implements AutoCloseable {
   private final FileChannel channel;
   private final FileLock lock;
 
-  private DiskCacheLock(FileChannel channel, FileLock lock) {
+  private FileSystemLock(FileChannel channel, FileLock lock) {
     this.channel = channel;
     this.lock = lock;
   }
 
   /**
-   * Acquires shared access to the disk cache.
+   * Acquires shared access to the lock file.
    *
    * @param path the path to the lock file
    * @throws IOException if an error occurred, including the lock currently being exclusively held
    *     by another process
    */
-  public static DiskCacheLock getShared(Path path) throws IOException {
+  public static FileSystemLock getShared(Path path) throws IOException {
     return get(path, true);
   }
 
   /**
-   * Acquires exclusive access to the disk cache.
+   * Acquires exclusive access to the lock file.
    *
    * @param path the path to the lock file
    * @throws IOException if an error occurred, including the lock currently being exclusively held
    *     by another process
    */
-  public static DiskCacheLock getExclusive(Path path) throws IOException {
+  public static FileSystemLock getExclusive(Path path) throws IOException {
     return get(path, false);
   }
 
-  private static DiskCacheLock get(Path path, boolean shared) throws IOException {
+  private static FileSystemLock get(Path path, boolean shared) throws IOException {
     path.getParentDirectory().createDirectoryAndParents();
     FileChannel channel =
         FileChannel.open(
@@ -67,9 +69,10 @@ private static DiskCacheLock get(Path path, boolean shared) throws IOException {
     FileLock lock = channel.tryLock(0, Long.MAX_VALUE, shared);
     if (lock == null) {
       throw new IOException(
-          "failed to acquire %s disk cache lock".formatted(shared ? "shared" : "exclusive"));
+          "failed to acquire %s filesystem lock on %s"
+              .formatted(shared ? "shared" : "exclusive", path));
     }
-    return new DiskCacheLock(channel, lock);
+    return new FileSystemLock(channel, lock);
   }
 
   private static String getPathStringForJavaIo(Path path) {
@@ -88,7 +91,7 @@ boolean isExclusive() {
     return !isShared();
   }
 
-  /** Releases access to the disk cache. */
+  /** Releases access to the lock file. */
   @Override
   public void close() throws IOException {
     try {

diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/BUILD b/src/test/java/com/google/devtools/build/lib/remote/disk/BUILD
@@ -13,42 +13,27 @@ filegroup(
     visibility = ["//src:__subpackages__"],
 )
 
-java_binary(
-    name = "external_lock_helper",
-    testonly = True,
-    srcs = ["ExternalLockHelper.java"],
-    jvm_flags = [
-        # Prevent the JVM from polluting stdout and interfere with communication with the parent.
-        "-Xlog:disable",
-        "-Xlog:all=warning:stderr",
-    ],
-    main_class = "com.google.devtools.build.lib.remote.disk.ExternalLockHelper",
-)
-
 java_test(
     name = "disk",
     srcs = glob(["*.java"]),
-    data = [":external_lock_helper"],
     test_class = "com.google.devtools.build.lib.AllTests",
     deps = [
         "//src/main/java/com/google/devtools/build/lib/remote:store",
         "//src/main/java/com/google/devtools/build/lib/remote/common",
         "//src/main/java/com/google/devtools/build/lib/remote/common:cache_not_found_exception",
         "//src/main/java/com/google/devtools/build/lib/remote/disk",
         "//src/main/java/com/google/devtools/build/lib/remote/util",
-        "//src/main/java/com/google/devtools/build/lib/shell",
-        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs/bazel",
         "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
         "//src/test/java/com/google/devtools/build/lib:test_runner",
         "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
+        "//src/test/java/com/google/devtools/build/lib/testutil:external_file_system_lock",
         "//third_party:error_prone_annotations",
         "//third_party:guava",
         "//third_party:junit4",
         "//third_party:mockito",
         "//third_party:truth",
-        "@bazel_tools//tools/java/runfiles",
         "@protobuf//:protobuf_java",
         "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",
     ],

diff --git a/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java b/src/test/java/com/google/devtools/build/lib/remote/disk/DiskCacheGarbageCollectorTest.java
@@ -19,6 +19,7 @@
 
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.remote.disk.DiskCacheGarbageCollector.CollectionStats;
+import com.google.devtools.build.lib.testutil.ExternalFileSystemLock;
 import com.google.devtools.build.lib.testutil.TestUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
@@ -182,12 +183,12 @@ public void ignoresTmpAndGcSubdirectories() throws Exception {
 
   @Test
   public void failsWhenLockIsAlreadyHeld() throws Exception {
-    try (var externalLock = ExternalLock.getShared(rootDir.getRelative("gc/lock"))) {
+    try (var externalLock = ExternalFileSystemLock.getShared(rootDir.getRelative("gc/lock"))) {
       Exception e =
           assertThrows(
               Exception.class, () -> runGarbageCollector(Optional.of(1L), Optional.empty()));
       assertThat(e).isInstanceOf(IOException.class);
-      assertThat(e).hasMessageThat().contains("failed to acquire exclusive disk cache lock");
+      assertThat(e).hasMessageThat().contains("failed to acquire exclusive filesystem lock");
     }
   }
 

diff --git a/src/test/java/com/google/devtools/build/lib/testutil/BUILD b/src/test/java/com/google/devtools/build/lib/testutil/BUILD
@@ -133,6 +133,30 @@ java_library(
     ],
 )
 
+java_library(
+    name = "external_file_system_lock",
+    srcs = ["ExternalFileSystemLock.java"],
+    data = [":external_file_system_lock_helper"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/shell",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//third_party:guava",
+        "@bazel_tools//tools/java/runfiles",
+    ],
+)
+
+java_binary(
+    name = "external_file_system_lock_helper",
+    srcs = ["ExternalFileSystemLockHelper.java"],
+    jvm_flags = [
+        # Prevent the JVM from polluting stdout and interfere with communication with the parent.
+        "-Xlog:disable",
+        "-Xlog:all=warning:stderr",
+    ],
+    main_class = "com.google.devtools.build.lib.testutil.ExternalFileSystemLockHelper",
+)
+
 java_library(
     name = "spawn_controller",
     srcs = ["SpawnController.java"],

diff --git a/...s/build/lib/remote/disk/ExternalLock.java → .../lib/testutil/ExternalFileSystemLock.java b/...s/build/lib/remote/disk/ExternalLock.java → .../lib/testutil/ExternalFileSystemLock.java
@@ -11,7 +11,7 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-package com.google.devtools.build.lib.remote.disk;
+package com.google.devtools.build.lib.testutil;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.shell.Subprocess;
@@ -26,22 +26,22 @@
  *
  * <p>This is needed for testing because the JVM does not allow overlapping locks.
  */
-public class ExternalLock implements AutoCloseable {
+public class ExternalFileSystemLock implements AutoCloseable {
   private static final String HELPER_PATH =
-      "io_bazel/src/test/java/com/google/devtools/build/lib/remote/disk/external_lock_helper"
+      "io_bazel/src/test/java/com/google/devtools/build/lib/testutil/external_file_system_lock_helper"
           + (OS.getCurrent() == OS.WINDOWS ? ".exe" : "");
 
     private final Subprocess subprocess;
 
-    static ExternalLock getShared(Path lockPath) throws IOException {
-      return new ExternalLock(lockPath, true);
+  public static ExternalFileSystemLock getShared(Path lockPath) throws IOException {
+    return new ExternalFileSystemLock(lockPath, true);
     }
 
-    static ExternalLock getExclusive(Path lockPath) throws IOException {
-      return new ExternalLock(lockPath, false);
+  public static ExternalFileSystemLock getExclusive(Path lockPath) throws IOException {
+    return new ExternalFileSystemLock(lockPath, false);
     }
 
-    ExternalLock(Path lockPath, boolean shared) throws IOException {
+  private ExternalFileSystemLock(Path lockPath, boolean shared) throws IOException {
       String binaryPath = Runfiles.preload().withSourceRepository("").rlocation(HELPER_PATH);
       this.subprocess =
           new SubprocessBuilder()

diff --git a/...d/lib/remote/disk/ExternalLockHelper.java → ...estutil/ExternalFileSystemLockHelper.java b/...d/lib/remote/disk/ExternalLockHelper.java → ...estutil/ExternalFileSystemLockHelper.java
@@ -11,7 +11,7 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-package com.google.devtools.build.lib.remote.disk;
+package com.google.devtools.build.lib.testutil;
 
 import java.io.IOException;
 import java.nio.channels.FileChannel;
@@ -20,9 +20,13 @@
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
 
-/** A helper binary that holds a shared or exclusive lock on a file. */
-public final class ExternalLockHelper {
-  private ExternalLockHelper() {}
+/**
+ * A helper binary that holds a shared or exclusive lock on a file.
+ *
+ * <p>Do not use this directly in a test. Use {@link ExternalFileSystemLock} instead.
+ */
+public final class ExternalFileSystemLockHelper {
+  private ExternalFileSystemLockHelper() {}
 
   public static void main(String[] args) throws IOException, InterruptedException {
     if (args.length != 2) {

diff --git a/src/test/java/com/google/devtools/build/lib/util/BUILD b/src/test/java/com/google/devtools/build/lib/util/BUILD
@@ -54,6 +54,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/util:crash_failure_details",
         "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
         "//src/main/java/com/google/devtools/build/lib/util:exit_code",
+        "//src/main/java/com/google/devtools/build/lib/util:file_system_lock",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
         "//src/main/java/com/google/devtools/build/lib/util:hash_codes",
         "//src/main/java/com/google/devtools/build/lib/util:interrupted_failure_details",
@@ -74,6 +75,7 @@ java_test(
         "//src/test/java/com/google/devtools/build/lib/testutil",
         "//src/test/java/com/google/devtools/build/lib/testutil:TestThread",
         "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
+        "//src/test/java/com/google/devtools/build/lib/testutil:external_file_system_lock",
         "//src/test/java/com/google/devtools/build/lib/util/subjects",
         "//third_party:guava",
         "//third_party:guava-testlib",

diff --git a/...ld/lib/remote/disk/DiskCacheLockTest.java → ...ls/build/lib/util/FileSystemLockTest.java b/...ld/lib/remote/disk/DiskCacheLockTest.java → ...ls/build/lib/util/FileSystemLockTest.java
@@ -11,11 +11,12 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-package com.google.devtools.build.lib.remote.disk;
+package com.google.devtools.build.lib.util;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
 
+import com.google.devtools.build.lib.testutil.ExternalFileSystemLock;
 import com.google.devtools.build.lib.testutil.TestUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
@@ -24,9 +25,9 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/** Tests for {@link DiskCacheLock}. */
+/** Tests for {@link FileSystemLock}. */
 @RunWith(JUnit4.class)
-public final class DiskCacheLockTest {
+public final class FileSystemLockTest {
 
   private Path lockPath;
 
@@ -38,47 +39,47 @@ public void setUp() throws Exception {
 
   @Test
   public void getShared_whenNotLocked_succeeds() throws Exception {
-    try (var lock = DiskCacheLock.getShared(lockPath)) {
+    try (var lock = FileSystemLock.getShared(lockPath)) {
       assertThat(lock.isShared()).isTrue();
     }
   }
 
   @Test
   public void getShared_whenLockedForSharedUse_succeeds() throws Exception {
-    try (var externalLock = ExternalLock.getShared(lockPath);
-        var lock = DiskCacheLock.getShared(lockPath)) {
+    try (var externalLock = ExternalFileSystemLock.getShared(lockPath);
+        var lock = FileSystemLock.getShared(lockPath)) {
       assertThat(lock.isShared()).isTrue();
     }
   }
 
   @Test
   public void getShared_whenLockedForExclusiveUse_fails() throws Exception {
-    try (var externalLock = ExternalLock.getExclusive(lockPath)) {
-      IOException e = assertThrows(IOException.class, () -> DiskCacheLock.getShared(lockPath));
-      assertThat(e).hasMessageThat().contains("failed to acquire shared disk cache lock");
+    try (var externalLock = ExternalFileSystemLock.getExclusive(lockPath)) {
+      IOException e = assertThrows(IOException.class, () -> FileSystemLock.getShared(lockPath));
+      assertThat(e).hasMessageThat().contains("failed to acquire shared filesystem lock");
     }
   }
 
   @Test
   public void getExclusive_whenNotLocked_succeeds() throws Exception {
-    try (var lock = DiskCacheLock.getExclusive(lockPath)) {
+    try (var lock = FileSystemLock.getExclusive(lockPath)) {
       assertThat(lock.isExclusive()).isTrue();
     }
   }
 
   @Test
   public void getExclusive_whenLockedForSharedUse_fails() throws Exception {
-    try (var externalLock = ExternalLock.getShared(lockPath)) {
-      IOException e = assertThrows(IOException.class, () -> DiskCacheLock.getExclusive(lockPath));
-      assertThat(e).hasMessageThat().contains("failed to acquire exclusive disk cache lock");
+    try (var externalLock = ExternalFileSystemLock.getShared(lockPath)) {
+      IOException e = assertThrows(IOException.class, () -> FileSystemLock.getExclusive(lockPath));
+      assertThat(e).hasMessageThat().contains("failed to acquire exclusive filesystem lock");
     }
   }
 
   @Test
   public void getExclusive_whenLockedForExclusiveUse_fails() throws Exception {
-    try (var lock = ExternalLock.getExclusive(lockPath)) {
-      IOException e = assertThrows(IOException.class, () -> DiskCacheLock.getExclusive(lockPath));
-      assertThat(e).hasMessageThat().contains("failed to acquire exclusive disk cache lock");
+    try (var lock = ExternalFileSystemLock.getExclusive(lockPath)) {
+      IOException e = assertThrows(IOException.class, () -> FileSystemLock.getExclusive(lockPath));
+      assertThat(e).hasMessageThat().contains("failed to acquire exclusive filesystem lock");
     }
   }
 }