Check for lock owning process when trying to fix corrupt packages

org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManager.java

@@ -242,13 +242,15 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
   protected void cleanUpCorruptPackages() throws IOException {
     for (File file : Objects.requireNonNull(cacheFolder.listFiles())) {
       if (file.getName().endsWith(".lock")) {
-        String packageDirectoryName = file.getName().substring(0, file.getName().length() - 5);
-        File packageDirectory = ManagedFileAccess.file(Utilities.path(cacheFolder, packageDirectoryName));
-        if (packageDirectory.exists()) {
-          Utilities.clearDirectory(packageDirectory.getAbsolutePath());
-          packageDirectory.delete();
+        if (locks.getCacheLock().canLockFileBeHeldByThisProcess(file)) {
+          String packageDirectoryName = file.getName().substring(0, file.getName().length() - 5);
+          File packageDirectory = ManagedFileAccess.file(Utilities.path(cacheFolder, packageDirectoryName));
+          if (packageDirectory.exists()) {
+            Utilities.clearDirectory(packageDirectory.getAbsolutePath());
+            packageDirectory.delete();
+          }
+          file.delete();
         }
-        file.delete();
       }
     }
   }

org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManagerLocks.java

@@ -108,6 +108,19 @@ public class FilesystemPackageCacheManagerLocks {
       }
       return result;
     }
+
+    public boolean canLockFileBeHeldByThisProcess(File lockFile) throws IOException {
+      return doWriteWithLock(() -> {
+      try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) {
+        FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
+        if (fileLock != null) {
+          fileLock.release();
+          channel.close();
+          return true;
+        }
+      }
+      return false;});
+    }
   }
 
   public class PackageLock {
@@ -225,6 +238,9 @@ public class FilesystemPackageCacheManagerLocks {
       cacheLock.getLock().writeLock().lock();
       lock.writeLock().lock();
 
+       /*TODO Eventually, this logic should exist in a Lockfile class so that it isn't duplicated between the main code and
+          the test code.
+        */
       try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) {
 
         FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false);

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerLockTests.java

@@ -183,8 +183,8 @@ public class FilesystemPackageManagerLockTests {
     FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager;
     final FilesystemPackageCacheManagerLocks.PackageLock packageLock = shorterTimeoutManager.getPackageLock(DUMMY_PACKAGE);
     File lockFile = getPackageLockFile();
-    Thread lockThread = LockfileUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5);
-    LockfileUtility.waitForLockfileCreation(cachePath,lockFile.getName());
+    Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5);
+    LockfileTestUtility.waitForLockfileCreation(cachePath,lockFile.getName());
 
     Exception exception = assertThrows(IOException.class, () -> {
       packageLock.doReadWithLock(() -> {
@@ -206,8 +206,8 @@ public class FilesystemPackageManagerLockTests {
 
     final File lockFile = getPackageLockFile();
 
-    Thread lockThread = LockfileUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5);
-    LockfileUtility.waitForLockfileCreation(cachePath,lockFile.getName());
+    Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5);
+    LockfileTestUtility.waitForLockfileCreation(cachePath,lockFile.getName());
 
     packageLock.doReadWithLock(() -> {
       assertThat(lockFile).doesNotExist();

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerTests.java

@@ -14,7 +14,6 @@ import java.util.List;
 import java.util.Random;
 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -118,6 +117,23 @@ public class FilesystemPackageManagerTests {
     assertEquals( System.getenv("ProgramData") + "\\.fhir\\packages", folder.getAbsolutePath());
   }
 
+  @Test
+  public void testCorruptPackageCleanup() throws IOException {
+    File cacheDirectory = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest"));
+
+    File dummyPackage = createDummyPackage(cacheDirectory, "example.fhir.uv.myig", "1.2.3");
+    File dummyLockFile = createDummyLockFile(cacheDirectory, "example.fhir.uv.myig" , "1.2.3");
+
+    assertThat(dummyPackage).isDirectory();
+    assertThat(dummyPackage).exists();
+    assertThat(dummyLockFile).exists();
+
+    FilesystemPackageCacheManager filesystemPackageCacheManager = new FilesystemPackageCacheManager.Builder().withCacheFolder(cacheDirectory.getAbsolutePath()).build();
+
+    assertThat(dummyPackage).doesNotExist();
+    assertThat(dummyLockFile).doesNotExist();
+  }
+
   @Test
   public void testTimeoutForLockedPackageRead() throws IOException, InterruptedException, TimeoutException {
     String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath();
@@ -129,11 +145,11 @@ public class FilesystemPackageManagerTests {
 
     Assertions.assertTrue(pcm.listPackages().isEmpty());
 
-    Thread lockThread = LockfileUtility.lockWaitAndDeleteInNewProcess(pcmPath, "example.fhir.uv.myig#1.2.3.lock", 10);
+    Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(pcmPath, "example.fhir.uv.myig#1.2.3.lock", 10);
     File directory = ManagedFileAccess.file(pcmPath, "example.fhir.uv.myig#1.2.3" );
     directory.mkdir();
 
-    LockfileUtility.waitForLockfileCreation(pcmPath, "example.fhir.uv.myig#1.2.3.lock");
+    LockfileTestUtility.waitForLockfileCreation(pcmPath, "example.fhir.uv.myig#1.2.3.lock");
 
     IOException exception = assertThrows(IOException.class, () -> pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3"));
 
@@ -142,10 +158,6 @@ public class FilesystemPackageManagerTests {
     lockThread.join();
   }
 
-
-
-
-
   @Test
   public void testReadFromCacheOnlyWaitsForLockDelete() throws IOException, InterruptedException, TimeoutException {
     String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath();
@@ -163,9 +175,8 @@ public class FilesystemPackageManagerTests {
     File directory = ManagedFileAccess.file(pcmPath, packageAndVersion);
     directory.mkdir();
 
-    Thread lockThread = LockfileUtility.lockWaitAndDeleteInNewProcess(pcmPath, "example.fhir.uv.myig#1.2.3.lock", 5);
-    LockfileUtility.waitForLockfileCreation(pcmPath, "example.fhir.uv.myig#1.2.3.lock");
-
+    Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(pcmPath, "example.fhir.uv.myig#1.2.3.lock", 5);
+    LockfileTestUtility.waitForLockfileCreation(pcmPath, "example.fhir.uv.myig#1.2.3.lock");
 
     NpmPackage npmPackage = pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3");
 
@@ -187,16 +198,16 @@ public class FilesystemPackageManagerTests {
     return params.stream();
   }
 
-  private void createDummyTemp(File cacheDirectory, String lowerCase) throws IOException {
-    createDummyPackage(cacheDirectory, lowerCase);
+  private File createDummyTemp(File cacheDirectory, String lowerCase) throws IOException {
+    return createDummyPackage(cacheDirectory, lowerCase);
   }
 
-  private void createDummyPackage(File cacheDirectory, String packageName, String packageVersion) throws IOException {
+  private File createDummyPackage(File cacheDirectory, String packageName, String packageVersion) throws IOException {
     String directoryName = packageName + "#" + packageVersion;
-    createDummyPackage(cacheDirectory, directoryName);
+    return createDummyPackage(cacheDirectory, directoryName);
   }
 
-  private static void createDummyPackage(File cacheDirectory, String directoryName) throws IOException {
+  private static File createDummyPackage(File cacheDirectory, String directoryName) throws IOException {
     File packageDirectory = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), directoryName);
     packageDirectory.mkdirs();
 
@@ -205,6 +216,16 @@ public class FilesystemPackageManagerTests {
     wr.write("Ain't nobody here but us chickens");
     wr.flush();
     wr.close();
+    return packageDirectory;
+  }
+
+  private File createDummyLockFile(File cacheDirectory, String packageName, String packageVersion) throws IOException {
+    final File dummyLockFile = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), packageName + "#" + packageVersion + ".lock");
+    final FileWriter wr = new FileWriter(dummyLockFile);
+    wr.write("Ain't nobody here but us chickens");
+    wr.flush();
+    wr.close();
+    return dummyLockFile;
   }
 
   private void assertThatDummyTempExists(File cacheDirectory, String dummyTempPackage) throws IOException {

org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java

@@ -10,7 +10,31 @@ import java.nio.file.*;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
-public class LockfileUtility {
+/**
+ * FilesystemPackageCacheManagerLocks relies on the existence of .lock files to prevent access to packages being written
+ * by processes outside the current JVM. Testing this functionality means creating a process outside the JUnit test JVM,
+ * which is achieved by running a separate Java process.
+ * <p/>
+ * Intended usage:
+ * <p/>
+ * The helper method {@link #lockWaitAndDeleteInNewProcess(String, String, int)} is the intended starting point for
+ * using this class.
+ * <p/>
+ *
+ *
+ * This class deliberately avoids using any dependencies outside java.*, which avoids having to construct a classpath
+ * for the separate process.
+ */
+public class LockfileTestUtility {
+
+  /**
+   * Main method to allow running this class.
+   *
+   * It is not recommended to call this method directly. Instead, use the provided {@link LockfileTestUtility.l} method.
+   *
+   *
+   * @param args
+    */
   public static void main(String[] args) {
     String lockFileName = args[1];
     String path = args[0];
@@ -24,6 +48,18 @@ public class LockfileUtility {
 
   }
 
+  /**
+   * Wait for the lock file to be created in the given path.
+   * <p/>
+   * Normally, within the same JVM, you could use a CountdownLatch for the same purpose, but since this the lock file is
+   * being created in a separate process, we need to use a mechanism that doesn't rely on shared threads.
+   *
+   * @param path The path containing the lock file
+   * @param lockFileName The name of the lock file
+   * @throws InterruptedException If the thread is interrupted while waiting
+   * @throws IOException If there is an error accessing the file system
+   * @throws TimeoutException If the lock file is not created within 10 seconds
+   */
   public static void waitForLockfileCreation(String path, String lockFileName) throws InterruptedException, IOException, TimeoutException {
     if (Files.exists(Paths.get(path, lockFileName))) {
       return;
@@ -50,9 +86,21 @@ public class LockfileUtility {
     }
   }
 
+  /**
+   * Static helper method that starts a new process, creates a lock file in the path and waits for a specified number of
+   * seconds before deleting it.
+   * <p/>
+   * This method calls the {@link #main(String[])} method in a new process.
+   *
+   * @param path The path to create the lockfile in
+   * @param lockFileName The name of the lockfile
+   * @param seconds The number of seconds to wait before deleting the lockfile
+   * @return The thread wrapping the process execution. This can be used to wait for the process to complete, so that
+   * System.out and System.err can be processed before tests return results.
+   */
   public static Thread lockWaitAndDeleteInNewProcess(String path, String lockFileName, int seconds)  {
     Thread t = new Thread(() -> {
-      ProcessBuilder processBuilder = new ProcessBuilder("java", "-cp", "target/test-classes:.", "org.hl7.fhir.utilities.npm.LockfileUtility", path, lockFileName, Integer.toString(seconds));
+      ProcessBuilder processBuilder = new ProcessBuilder("java", "-cp", "target/test-classes:.", LockfileTestUtility.class.getName(), path, lockFileName, Integer.toString(seconds));
       try {
         Process process = processBuilder.start();
         process.getErrorStream().transferTo(System.err);
@@ -66,6 +114,21 @@ public class LockfileUtility {
     return t;
   }
 
+  /**
+   * The actual logic to create a .lock file.
+   * <p/>
+   * This should match the logic in FilesystemPackageCacheManagerLocks
+   * <p/>
+   *
+   * @param path The path to create the lockfile in
+   * @param lockFileName The name of the lockfile
+   * @param seconds The number of seconds to wait before deleting the lockfile
+   * @throws InterruptedException If the thread is interrupted while waiting
+   * @throws IOException If there is an error accessing the file system
+   */
+  /* TODO Eventually, this logic should exist in a Lockfile class so that it isn't duplicated between the main code and
+      the test code.
+   */
   private static void lockWaitAndDelete(String path, String lockFileName, int seconds) throws InterruptedException, IOException {
 
     File file = Paths.get(path,lockFileName).toFile();