From 48066859d5b15e1b1d79e107b30dab37302ab04f Mon Sep 17 00:00:00 2001 From: dotasek Date: Wed, 18 Sep 2024 11:30:59 -0400 Subject: [PATCH] Check for lock owning process when trying to fix corrupt packages --- .../npm/FilesystemPackageCacheManager.java | 14 ++-- .../FilesystemPackageCacheManagerLocks.java | 16 +++++ .../FilesystemPackageManagerLockTests.java | 8 +-- .../npm/FilesystemPackageManagerTests.java | 51 +++++++++----- ...eUtility.java => LockfileTestUtility.java} | 67 ++++++++++++++++++- 5 files changed, 129 insertions(+), 27 deletions(-) rename org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/{LockfileUtility.java => LockfileTestUtility.java} (52%) diff --git a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManager.java b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManager.java index 71f11178e..f3fba6551 100644 --- a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManager.java +++ b/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(); } } } diff --git a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManagerLocks.java b/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManagerLocks.java index 85b852da3..582596420 100644 --- a/org.hl7.fhir.utilities/src/main/java/org/hl7/fhir/utilities/npm/FilesystemPackageCacheManagerLocks.java +++ b/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); diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerLockTests.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerLockTests.java index a597fbacb..e20ecd747 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerLockTests.java +++ b/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(); diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerTests.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerTests.java index 5cfb1ae2a..d7565a348 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/FilesystemPackageManagerTests.java +++ b/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 { diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java similarity index 52% rename from org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileUtility.java rename to org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java index 5d4bedd10..9ea70fda0 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileUtility.java +++ b/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. + *

+ * Intended usage: + *

+ * The helper method {@link #lockWaitAndDeleteInNewProcess(String, String, int)} is the intended starting point for + * using this class. + *

+ * + * + * 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. + *

+ * 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. + *

+ * 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. + *

+ * This should match the logic in FilesystemPackageCacheManagerLocks + *

+ * + * @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();