From e001d781b94bb38f67a3f0c56c37047d0a3864e4 Mon Sep 17 00:00:00 2001 From: dotasek Date: Mon, 9 Sep 2024 17:03:56 -0400 Subject: [PATCH 01/11] WIP tests for lock file cleanup --- .../npm/FilesystemPackageCacheManager.java | 19 +++++ .../FilesystemPackageCacheManagerLocks.java | 14 ++++ .../npm/FilesystemPackageManagerTests.java | 82 ++++++++++++++++++- 3 files changed, 111 insertions(+), 4 deletions(-) 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 df192cc19..37e55c020 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 @@ -215,12 +215,31 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple createIniFile(); } else { deleteOldTempDirectories(); + cleanUpCorruptPackages(); } } return null; }); } + /* + Look for .lock files that are not actively held by a process. If found, delete the lock file, and the package + referenced. + */ + 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(); + } + file.delete(); + } + } + } + private boolean isCacheFolderValid() throws IOException { String iniPath = getPackagesIniPath(); File iniFile = ManagedFileAccess.file(iniPath); 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 d674c750e..fd75bde5d 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 @@ -118,6 +118,20 @@ public class FilesystemPackageCacheManagerLocks { if (!lockFile.exists()) { return; } + + // Check if the file is locked by a process. If it is not, it is likely an incomplete package cache install, and + // we should throw an exception. + if (lockFile.isFile()) { + try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) { + FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, true); + if (fileLock != null) { + fileLock.release(); + channel.close(); + throw new IOException("Lock file exists, but is not locked by a process: " + lockFile.getName()); + } + } + } + try (WatchService watchService = FileSystems.getDefault().newWatchService()) { Path dir = lockFile.getParentFile().toPath(); dir.register(watchService, StandardWatchEventKinds.ENTRY_DELETE); 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 96754c2b8..0e135af8d 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 @@ -1,22 +1,30 @@ package org.hl7.fhir.utilities.npm; -import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.junit.jupiter.api.Assertions.*; import java.io.File; import java.io.IOException; +import java.io.RandomAccessFile; +import java.nio.channels.FileChannel; +import java.nio.channels.FileLock; import java.nio.file.Files; import java.util.ArrayList; import java.util.List; import java.util.Random; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Stream; import javax.annotation.Nonnull; +import org.hl7.fhir.utilities.Utilities; import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; -import org.junit.jupiter.api.RepeatedTest; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.DisabledOnOs; import org.junit.jupiter.api.condition.EnabledOnOs; @@ -111,6 +119,67 @@ public class FilesystemPackageManagerTests { assertEquals( System.getenv("ProgramData") + "\\.fhir\\packages", folder.getAbsolutePath()); } + @Test + public void testFailureForUnlockedLockFiles() throws IOException, InterruptedException { + String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath(); + + final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder().withCacheFolder(pcmPath).build(); + + Assertions.assertTrue(pcm.listPackages().isEmpty()); + + //Now sneak in a new lock file and directory: + File lockFile = ManagedFileAccess.file(pcmPath, "example.fhir.uv.myig#1.2.3.lock"); + lockFile.createNewFile(); + File directory = ManagedFileAccess.file(pcmPath, "example.fhir.uv.myig#1.2.3" ); + directory.mkdir(); + + IOException exception = assertThrows(IOException.class, () -> pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3")); + assertThat(exception.getMessage()).contains("Lock file exists, but is not locked by a process"); + } + + @Test + //@EnabledOnOs(OS.LINUX) + public void testCacheCleanupForUnlockedLockFiles() throws IOException, InterruptedException { + String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath(); + + final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder().withCacheFolder(pcmPath).build(); + + Assertions.assertTrue(pcm.listPackages().isEmpty()); + + String packageAndVersion = "example.fhir.uv.myig#1.2.3"; + String lockFileName = packageAndVersion + ".lock"; + //Now sneak in a new lock file and directory: + File lockFile = ManagedFileAccess.file(pcmPath, lockFileName); + lockFile.createNewFile(); + File directory = ManagedFileAccess.file(pcmPath, packageAndVersion); + directory.mkdir(); + + // We can't create a lock file from within the same JVM, so we have to use the flock utility, which is OS dependent. + // The following works for Linux only. + ProcessBuilder processBuilder = new ProcessBuilder("flock", lockFileName, "--command", "sleep 10"); + processBuilder.directory(new File(pcmPath)); + processBuilder.start(); + + ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(); + + executorService.schedule(()->{ + try { + Utilities.clearDirectory(directory.getAbsolutePath()); + directory.delete(); + } catch (IOException e) { + throw new RuntimeException(e); + } + lockFile.delete(); + }, 15, TimeUnit.SECONDS); + + IOException ioException = assertThrows(IOException.class, () -> { pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3"); }); + + + ioException.printStackTrace(); + + + } + /** We repeat the same tests multiple times here, in order to catch very rare edge cases. */ @@ -133,7 +202,7 @@ public class FilesystemPackageManagerTests { Random rand = new Random(); final AtomicInteger totalSuccessful = new AtomicInteger(); - final ConcurrentHashMap successfulThreads = new ConcurrentHashMap(); + final ConcurrentHashMap successfulThreads = new ConcurrentHashMap<>(); List threads = new ArrayList<>(); for (int i = 0; i < threadTotal; i++) { final int index = i; @@ -142,22 +211,27 @@ public class FilesystemPackageManagerTests { System.out.println("Thread #" + index + ": " + Thread.currentThread().getId() + " started"); final int randomPCM = rand.nextInt(packageCacheManagerTotal); final int randomOperation = rand.nextInt(4); + final String operationName; if (packageCacheManagers[randomPCM] == null) { packageCacheManagers[randomPCM] = new FilesystemPackageCacheManager.Builder().withCacheFolder(pcmPath).build(); } FilesystemPackageCacheManager pcm = packageCacheManagers[randomPCM]; if (randomOperation == 0) { + operationName = "addPackageToCache"; pcm.addPackageToCache("example.fhir.uv.myig", "1.2.3", this.getClass().getResourceAsStream("/npm/dummy-package.tgz"), "https://packages.fhir.org/example.fhir.uv.myig/1.2.3"); } else if (randomOperation == 1) { + operationName = "clear"; pcm.clear(); } else if (randomOperation == 2) { + operationName = "loadPackageFromCacheOnly"; pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3"); } else { + operationName = "removePackage"; pcm.removePackage("example.fhir.uv.myig", "1.2.3"); } totalSuccessful.incrementAndGet(); successfulThreads.put(Thread.currentThread().getId(), index); - System.out.println("Thread #" + index + ": " + Thread.currentThread().getId() + " completed"); + System.out.println("Thread #" + index + ": " + Thread.currentThread().getId() + " completed. Ran: " + operationName); } catch (Exception e) { e.printStackTrace(); System.err.println("Thread #" + index + ": " + Thread.currentThread().getId() + " failed"); From f7cf74423c06356b5c3e3a9ba08bddeea0945a6d Mon Sep 17 00:00:00 2001 From: dotasek Date: Tue, 10 Sep 2024 17:07:27 -0400 Subject: [PATCH 02/11] WIP2 More tests on file lock --- .../FilesystemPackageCacheManagerLocks.java | 2 +- .../npm/FilesystemPackageManagerTests.java | 41 +++++++++-------- .../fhir/utilities/npm/LockfileUtility.java | 44 +++++++++++++++++++ 3 files changed, 67 insertions(+), 20 deletions(-) create mode 100644 org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileUtility.java 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 fd75bde5d..fbe47dc63 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 @@ -195,7 +195,7 @@ public class FilesystemPackageCacheManagerLocks { try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) { FileLock fileLock = null; while (fileLock == null) { - fileLock = channel.tryLock(0, Long.MAX_VALUE, true); + fileLock = channel.tryLock(0, Long.MAX_VALUE, false); if (fileLock == null) { Thread.sleep(100); // Wait and retry } 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 0e135af8d..745ae798f 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 @@ -137,6 +137,24 @@ public class FilesystemPackageManagerTests { assertThat(exception.getMessage()).contains("Lock file exists, but is not locked by a process"); } + private static Thread lockWaitAndDelete(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)); + try { + Process process = processBuilder.start(); + process.getErrorStream().transferTo(System.err); + process.getInputStream().transferTo(System.out); + process.waitFor(); + } catch (IOException | InterruptedException e) { + throw new RuntimeException(e); + } + }); + t.start(); + return t; + } + + + @Test //@EnabledOnOs(OS.LINUX) public void testCacheCleanupForUnlockedLockFiles() throws IOException, InterruptedException { @@ -150,34 +168,19 @@ public class FilesystemPackageManagerTests { String lockFileName = packageAndVersion + ".lock"; //Now sneak in a new lock file and directory: File lockFile = ManagedFileAccess.file(pcmPath, lockFileName); - lockFile.createNewFile(); + //lockFile.createNewFile(); File directory = ManagedFileAccess.file(pcmPath, packageAndVersion); directory.mkdir(); - // We can't create a lock file from within the same JVM, so we have to use the flock utility, which is OS dependent. - // The following works for Linux only. - ProcessBuilder processBuilder = new ProcessBuilder("flock", lockFileName, "--command", "sleep 10"); - processBuilder.directory(new File(pcmPath)); - processBuilder.start(); - - ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(); - - executorService.schedule(()->{ - try { - Utilities.clearDirectory(directory.getAbsolutePath()); - directory.delete(); - } catch (IOException e) { - throw new RuntimeException(e); - } - lockFile.delete(); - }, 15, TimeUnit.SECONDS); + Thread lockingThread = lockWaitAndDelete(pcmPath, lockFileName, 10); + Thread.sleep(200); IOException ioException = assertThrows(IOException.class, () -> { pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3"); }); ioException.printStackTrace(); - + lockingThread.join(); } /** 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/LockfileUtility.java new file mode 100644 index 000000000..c89321b08 --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileUtility.java @@ -0,0 +1,44 @@ +package org.hl7.fhir.utilities.npm; + +import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; + +import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.RandomAccessFile; +import java.nio.channels.FileChannel; +import java.nio.channels.FileLock; +import java.nio.file.Paths; + +public class LockfileUtility { + public static void main(String[] args) { + String lockFileName = args[1]; + String path = args[0]; + int seconds = Integer.parseInt(args[2]); + + try { + lockWaitAndDelete(path, lockFileName, seconds); + } catch (InterruptedException | IOException e) { + throw new RuntimeException(e); + } + + } + + private static void lockWaitAndDelete(String path, String lockFileName, int seconds) throws InterruptedException, IOException { + File file = Paths.get(path,lockFileName).toFile(); + try (FileChannel channel = new RandomAccessFile(file.getAbsolutePath(), "rw").getChannel()) { + FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false); + if (fileLock != null) { + System.out.println("File "+lockFileName+" is locked. Waiting for " + seconds + " seconds to release"); + Thread.sleep(seconds * 1000L); + fileLock.release(); + System.out.println("File "+lockFileName+" is released."); + + channel.close(); + }}finally { + if (file.exists()) { + file.delete(); + } + } + } +} From a05f471c9cee3cb5428ca3a1e461c93f211d5d39 Mon Sep 17 00:00:00 2001 From: dotasek Date: Fri, 13 Sep 2024 16:49:46 -0400 Subject: [PATCH 03/11] Allow injection of lock parameters + correct usage of locked file write --- .../npm/FilesystemPackageCacheManager.java | 42 +++++---- .../FilesystemPackageCacheManagerLocks.java | 93 +++++++++++++------ .../FilesystemPackageManagerLockTests.java | 57 ++++++------ .../npm/FilesystemPackageManagerTests.java | 49 ++++------ .../fhir/utilities/npm/LockfileUtility.java | 61 ++++++++++-- 5 files changed, 189 insertions(+), 113 deletions(-) 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 fbc09e90f..71f11178e 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 @@ -8,6 +8,7 @@ import java.text.SimpleDateFormat; import java.util.*; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import lombok.Getter; import lombok.Setter; @@ -77,12 +78,13 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple private final FilesystemPackageCacheManagerLocks locks; + private final FilesystemPackageCacheManagerLocks.LockParameters lockParameters; + // When running in testing mode, some packages are provided from the test case repository rather than by the normal means // the PackageProvider is responsible for this. if no package provider is defined, or it declines to handle the package, // then the normal means will be used public interface IPackageProvider { boolean handlesPackage(String id, String version); - InputStreamWithSrc provide(String id, String version) throws IOException; } @@ -92,6 +94,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple public static final String PACKAGE_VERSION_REGEX_OPT = "^[A-Za-z][A-Za-z0-9\\_\\-]*(\\.[A-Za-z0-9\\_\\-]+)+(\\#[A-Za-z0-9\\-\\_]+(\\.[A-Za-z0-9\\-\\_]+)*)?$"; private static final Logger ourLog = LoggerFactory.getLogger(FilesystemPackageCacheManager.class); private static final String CACHE_VERSION = "3"; // second version - see wiki page + @Nonnull private final File cacheFolder; @@ -100,6 +103,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple private final Map ciList = new HashMap<>(); private JsonArray buildInfo; private boolean suppressErrors; + @Setter @Getter private boolean minimalMemory; @@ -113,9 +117,20 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple @Getter private final List packageServers; + @With + @Getter + private final FilesystemPackageCacheManagerLocks.LockParameters lockParameters; + public Builder() throws IOException { this.cacheFolder = getUserCacheFolder(); this.packageServers = getPackageServersFromFHIRSettings(); + this.lockParameters = null; + } + + private Builder(File cacheFolder, List packageServers, FilesystemPackageCacheManagerLocks.LockParameters lockParameters) { + this.cacheFolder = cacheFolder; + this.packageServers = packageServers; + this.lockParameters = lockParameters; } private File getUserCacheFolder() throws IOException { @@ -143,17 +158,12 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple return PackageServer.getConfiguredServers(); } - private Builder(File cacheFolder, List packageServers) { - this.cacheFolder = cacheFolder; - this.packageServers = packageServers; - } - public Builder withCacheFolder(String cacheFolderPath) throws IOException { File cacheFolder = ManagedFileAccess.file(cacheFolderPath); if (!cacheFolder.exists()) { throw new FHIRException("The folder '" + cacheFolder + "' could not be found"); } - return new Builder(cacheFolder, this.packageServers); + return new Builder(cacheFolder, this.packageServers, this.lockParameters); } public Builder withSystemCacheFolder() throws IOException { @@ -163,11 +173,11 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple } else { systemCacheFolder = ManagedFileAccess.file(Utilities.path("/var", "lib", ".fhir", "packages")); } - return new Builder(systemCacheFolder, this.packageServers); + return new Builder(systemCacheFolder, this.packageServers, this.lockParameters); } public Builder withTestingCacheFolder() throws IOException { - return new Builder(ManagedFileAccess.file(Utilities.path("[tmp]", ".fhir", "packages")), this.packageServers); + return new Builder(ManagedFileAccess.file(Utilities.path("[tmp]", ".fhir", "packages")), this.packageServers, this.lockParameters); } public FilesystemPackageCacheManager build() throws IOException { @@ -181,15 +191,15 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple throw e; } } - return new FilesystemPackageCacheManager(cacheFolder, packageServers, locks); + return new FilesystemPackageCacheManager(cacheFolder, packageServers, locks, lockParameters); } } - private FilesystemPackageCacheManager(@Nonnull File cacheFolder, @Nonnull List packageServers, @Nonnull FilesystemPackageCacheManagerLocks locks) throws IOException { + private FilesystemPackageCacheManager(@Nonnull File cacheFolder, @Nonnull List packageServers, @Nonnull FilesystemPackageCacheManagerLocks locks, @Nullable FilesystemPackageCacheManagerLocks.LockParameters lockParameters) throws IOException { super(packageServers); this.cacheFolder = cacheFolder; this.locks = locks; - + this.lockParameters = lockParameters; prepareCacheFolder(); } @@ -441,7 +451,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple } return null; - }); + }, lockParameters); } /** @@ -485,7 +495,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple return null; } return loadPackageInfo(path); - }); + }, lockParameters); if (foundPackage != null) { if (foundPackage.isIndexed()){ return foundPackage; @@ -508,7 +518,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple String path = Utilities.path(cacheFolder, foundPackageFolder); output.checkIndexed(path); return output; - }); + }, lockParameters); } } } @@ -609,7 +619,7 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple throw e; } return npmPackage; - }); + }, lockParameters); } private void log(String s) { 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 bb68ae248..85b852da3 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 @@ -1,14 +1,18 @@ package org.hl7.fhir.utilities.npm; import lombok.Getter; -import org.hl7.fhir.utilities.TextFile; +import lombok.With; import org.hl7.fhir.utilities.Utilities; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.io.File; import java.io.IOException; import java.io.RandomAccessFile; +import java.nio.ByteBuffer; import java.nio.channels.FileChannel; import java.nio.channels.FileLock; +import java.nio.charset.StandardCharsets; import java.nio.file.*; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; @@ -27,9 +31,23 @@ public class FilesystemPackageCacheManagerLocks { private final File cacheFolder; - private final Long lockTimeoutTime; + private static final LockParameters lockParameters = new LockParameters(); - private final TimeUnit lockTimeoutTimeUnit; + public static class LockParameters { + @Getter @With + private final long lockTimeoutTime; + @Getter @With + private final TimeUnit lockTimeoutTimeUnit; + + public LockParameters() { + this(60L, TimeUnit.SECONDS); + } + + public LockParameters(long lockTimeoutTime, TimeUnit lockTimeoutTimeUnit) { + this.lockTimeoutTime = lockTimeoutTime; + this.lockTimeoutTimeUnit = lockTimeoutTimeUnit; + } + } /** * This method is intended to be used only for testing purposes. @@ -43,21 +61,9 @@ public class FilesystemPackageCacheManagerLocks { * @throws IOException */ public FilesystemPackageCacheManagerLocks(File cacheFolder) throws IOException { - this(cacheFolder, 60L, TimeUnit.SECONDS); - } - - private FilesystemPackageCacheManagerLocks(File cacheFolder, Long lockTimeoutTime, TimeUnit lockTimeoutTimeUnit) throws IOException { this.cacheFolder = cacheFolder; - this.lockTimeoutTime = lockTimeoutTime; - this.lockTimeoutTimeUnit = lockTimeoutTimeUnit; } - /** - * This method is intended to be used only for testing purposes. - */ - protected FilesystemPackageCacheManagerLocks withLockTimeout(Long lockTimeoutTime, TimeUnit lockTimeoutTimeUnit) throws IOException { - return new FilesystemPackageCacheManagerLocks(cacheFolder, lockTimeoutTime, lockTimeoutTimeUnit); - } /** * Returns a single FilesystemPackageCacheManagerLocks instance for the given cacheFolder. @@ -114,7 +120,7 @@ public class FilesystemPackageCacheManagerLocks { this.lock = lock; } - private void checkForLockFileWaitForDeleteIfExists(File lockFile) throws IOException { + private void checkForLockFileWaitForDeleteIfExists(File lockFile, @Nonnull LockParameters lockParameters) throws IOException { if (!lockFile.exists()) { return; } @@ -129,10 +135,11 @@ public class FilesystemPackageCacheManagerLocks { channel.close(); throw new IOException("Lock file exists, but is not locked by a process: " + lockFile.getName()); } + System.out.println("File is locked."); } } try { - waitForLockFileDeletion(lockFile); + waitForLockFileDeletion(lockFile, lockParameters); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new IOException("Thread interrupted while waiting for lock", e); @@ -143,12 +150,13 @@ public class FilesystemPackageCacheManagerLocks { Wait for the lock file to be deleted. If the lock file is not deleted within the timeout or if the thread is interrupted, an IOException is thrown. */ - private void waitForLockFileDeletion(File lockFile) throws IOException, InterruptedException { + private void waitForLockFileDeletion(File lockFile, @Nonnull LockParameters lockParameters) throws IOException, InterruptedException { + try (WatchService watchService = FileSystems.getDefault().newWatchService()) { Path dir = lockFile.getParentFile().toPath(); dir.register(watchService, StandardWatchEventKinds.ENTRY_DELETE); - WatchKey key = watchService.poll(lockTimeoutTime, lockTimeoutTimeUnit); + WatchKey key = watchService.poll(lockParameters.lockTimeoutTime, lockParameters.lockTimeoutTimeUnit); if (key == null) { // It is possible that the lock file is deleted before the watch service is registered, so if we timeout at // this point, we should check if the lock file still exists. @@ -173,15 +181,27 @@ public class FilesystemPackageCacheManagerLocks { } - public T doReadWithLock(FilesystemPackageCacheManager.CacheLockFunction f) throws IOException { + /** + * Wraps the execution of a package read function with the appropriate cache, package, and .lock file locking and + * checks. + * + * @param function The function to execute + * @param lockParameters The parameters for the lock + * @return The return of the function + * @param The return type of the function + * @throws IOException If an error occurs while managing locking. + */ + public T doReadWithLock(FilesystemPackageCacheManager.CacheLockFunction function, @Nullable LockParameters lockParameters) throws IOException { + final LockParameters resolvedLockParameters = lockParameters != null ? lockParameters : FilesystemPackageCacheManagerLocks.lockParameters; + cacheLock.getLock().readLock().lock(); lock.readLock().lock(); - checkForLockFileWaitForDeleteIfExists(lockFile); + checkForLockFileWaitForDeleteIfExists(lockFile, resolvedLockParameters); T result = null; try { - result = f.get(); + result = function.get(); } finally { lock.readLock().unlock(); cacheLock.getLock().readLock().unlock(); @@ -189,7 +209,19 @@ public class FilesystemPackageCacheManagerLocks { return result; } - public T doWriteWithLock(FilesystemPackageCacheManager.CacheLockFunction f) throws IOException { + /** + * Wraps the execution of a package write function with the appropriate cache, package, and .lock file locking and + * checks. + * + * @param function The function to execute + * @param lockParameters The parameters for the lock + * @return The return of the function + * @param The return type of the function + * @throws IOException If an error occurs while managing locking. + */ + public T doWriteWithLock(FilesystemPackageCacheManager.CacheLockFunction function, @Nullable LockParameters lockParameters) throws IOException { + final LockParameters resolvedLockParameters = lockParameters != null ? lockParameters : FilesystemPackageCacheManagerLocks.lockParameters; + cacheLock.getLock().writeLock().lock(); lock.writeLock().lock(); @@ -198,19 +230,20 @@ public class FilesystemPackageCacheManagerLocks { FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false); if (fileLock == null) { - waitForLockFileDeletion(lockFile); + waitForLockFileDeletion(lockFile, resolvedLockParameters); + fileLock = channel.tryLock(0, Long.MAX_VALUE, false); + } + if (fileLock == null) { + throw new IOException("Failed to acquire lock on file: " + lockFile.getName()); } if (!lockFile.isFile()) { - try { - TextFile.stringToFile(String.valueOf(ProcessHandle.current().pid()), lockFile); - } catch (IOException e) { - throw new IOException("Error writing lock file.", e); - } + final ByteBuffer buff = ByteBuffer.wrap(String.valueOf(ProcessHandle.current().pid()).getBytes(StandardCharsets.UTF_8)); + channel.write(buff); } T result = null; try { - result = f.get(); + result = function.get(); } finally { fileLock.release(); channel.close(); 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 f1d71dda3..a597fbacb 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 @@ -1,6 +1,5 @@ package org.hl7.fhir.utilities.npm; -import org.hl7.fhir.utilities.TextFile; import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -12,6 +11,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -49,13 +49,13 @@ public class FilesystemPackageManagerLockTests { packageLock.doWriteWithLock(() -> { assertThat(packageLock.getLockFile()).exists(); return null; - }); + }, null); assertThat(packageLock.getLockFile()).doesNotExist(); packageLock.doReadWithLock(() -> { assertThat(packageLock.getLockFile()).doesNotExist(); return null; - }); + }, null); } @Test void testNoPackageWriteOrReadWhileWholeCacheIsLocked() throws IOException, InterruptedException { @@ -87,7 +87,7 @@ public class FilesystemPackageManagerLockTests { packageLock.doWriteWithLock(() -> { assertThat(cacheLockFinished.get()).isTrue(); return null; - }); + }, null); } catch (IOException e) { throw new RuntimeException(e); } @@ -97,7 +97,7 @@ public class FilesystemPackageManagerLockTests { packageLock.doReadWithLock(() -> { assertThat(cacheLockFinished.get()).isTrue(); return null; - }); + }, null); } catch (IOException e) { throw new RuntimeException(e); } @@ -133,7 +133,7 @@ public class FilesystemPackageManagerLockTests { assertThat(writeCount).isEqualTo(1); writeCounter.decrementAndGet(); return null; - }); + }, null); } catch (IOException e) { throw new RuntimeException(e); } @@ -156,7 +156,7 @@ public class FilesystemPackageManagerLockTests { } readCounter.decrementAndGet(); return null; - }); + }, null); } catch (IOException e) { throw new RuntimeException(e); } @@ -179,49 +179,46 @@ public class FilesystemPackageManagerLockTests { } @Test - public void testReadWhenLockedByFileTimesOut() throws IOException { - FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager.withLockTimeout(3L, TimeUnit.SECONDS); + public void testReadWhenLockedByFileTimesOut() throws IOException, InterruptedException, TimeoutException { + FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager; final FilesystemPackageCacheManagerLocks.PackageLock packageLock = shorterTimeoutManager.getPackageLock(DUMMY_PACKAGE); - File lockFile = createPackageLockFile(); + File lockFile = getPackageLockFile(); + Thread lockThread = LockfileUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5); + LockfileUtility.waitForLockfileCreation(cachePath,lockFile.getName()); Exception exception = assertThrows(IOException.class, () -> { packageLock.doReadWithLock(() -> { assertThat(lockFile).exists(); return null; - }); + }, new FilesystemPackageCacheManagerLocks.LockParameters(3L, TimeUnit.SECONDS)); }); - assertThat(exception.getMessage()).contains("Error reading package"); + assertThat(exception.getMessage()).contains("Package cache timed out waiting for lock"); assertThat(exception.getCause().getMessage()).contains("Timeout waiting for lock file deletion: " + lockFile.getName()); + + lockThread.join(); } @Test - public void testReadWhenLockFileIsDeleted() throws IOException { - FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager.withLockTimeout(5L, TimeUnit.SECONDS); - final FilesystemPackageCacheManagerLocks.PackageLock packageLock = shorterTimeoutManager.getPackageLock(DUMMY_PACKAGE); - File lockFile = createPackageLockFile(); + public void testReadWhenLockFileIsDeleted() throws IOException, InterruptedException, TimeoutException { - Thread t = new Thread(() -> { - try { - Thread.sleep(2000); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - lockFile.delete(); - }); - t.start(); + final FilesystemPackageCacheManagerLocks.PackageLock packageLock = filesystemPackageCacheLockManager.getPackageLock(DUMMY_PACKAGE); + + final File lockFile = getPackageLockFile(); + + Thread lockThread = LockfileUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5); + LockfileUtility.waitForLockfileCreation(cachePath,lockFile.getName()); packageLock.doReadWithLock(() -> { assertThat(lockFile).doesNotExist(); return null; - }); + }, new FilesystemPackageCacheManagerLocks.LockParameters(10L, TimeUnit.SECONDS)); + lockThread.join(); } - private File createPackageLockFile() throws IOException { - File lockFile = Path.of(cachePath, DUMMY_PACKAGE + ".lock").toFile(); - TextFile.stringToFile("", lockFile); - return lockFile; + private File getPackageLockFile() { + return Path.of(cachePath, DUMMY_PACKAGE + ".lock").toFile(); } } 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 e87c8d8f1..5cfb1ae2a 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 @@ -16,6 +16,7 @@ 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; import java.util.stream.Stream; @@ -118,44 +119,35 @@ public class FilesystemPackageManagerTests { } @Test - public void testTimeoutForLockedPackageRead() throws IOException, InterruptedException { + public void testTimeoutForLockedPackageRead() throws IOException, InterruptedException, TimeoutException { String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath(); - final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder().withCacheFolder(pcmPath).build(); + final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder() + .withCacheFolder(pcmPath) + .withLockParameters(new FilesystemPackageCacheManagerLocks.LockParameters(5,TimeUnit.SECONDS)) + .build(); Assertions.assertTrue(pcm.listPackages().isEmpty()); - //Now sneak in a new lock file and directory: - File lockFile = ManagedFileAccess.file(pcmPath, "example.fhir.uv.myig#1.2.3.lock"); - lockFile.createNewFile(); + Thread lockThread = LockfileUtility.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"); + IOException exception = assertThrows(IOException.class, () -> pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3")); - assertThat(exception.getMessage()).contains("Error reading package."); + + assertThat(exception.getMessage()).contains("Package cache timed out waiting for lock"); assertThat(exception.getCause().getMessage()).contains("Timeout waiting for lock file deletion"); + lockThread.join(); } - private static Thread lockWaitAndDelete(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)); - try { - Process process = processBuilder.start(); - process.getErrorStream().transferTo(System.err); - process.getInputStream().transferTo(System.out); - process.waitFor(); - } catch (IOException | InterruptedException e) { - throw new RuntimeException(e); - } - }); - t.start(); - return t; - } + @Test - public void testReadFromCacheOnlyWaitsForLockDelete() throws IOException, InterruptedException { + public void testReadFromCacheOnlyWaitsForLockDelete() throws IOException, InterruptedException, TimeoutException { String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath(); final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder().withCacheFolder(pcmPath).build(); @@ -166,23 +158,20 @@ public class FilesystemPackageManagerTests { String packageAndVersion = "example.fhir.uv.myig#1.2.3"; - String lockFileName = packageAndVersion + ".lock"; //Now sneak in a new lock file and directory: - File lockFile = ManagedFileAccess.file(pcmPath, lockFileName); - lockFile.createNewFile(); + File directory = ManagedFileAccess.file(pcmPath, packageAndVersion); directory.mkdir(); - final ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1); - executor.schedule(() -> { - lockFile.delete(); - }, 5, TimeUnit.SECONDS); + 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"); + NpmPackage npmPackage = pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3"); assertThat(npmPackage.id()).isEqualTo("example.fhir.uv.myig"); - + lockThread.join(); } /** 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/LockfileUtility.java index c89321b08..5d4bedd10 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/LockfileUtility.java @@ -1,14 +1,14 @@ package org.hl7.fhir.utilities.npm; -import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; -import java.io.File; -import java.io.FileNotFoundException; -import java.io.IOException; -import java.io.RandomAccessFile; +import java.io.*; +import java.nio.ByteBuffer; import java.nio.channels.FileChannel; import java.nio.channels.FileLock; -import java.nio.file.Paths; +import java.nio.charset.StandardCharsets; +import java.nio.file.*; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; public class LockfileUtility { public static void main(String[] args) { @@ -24,14 +24,61 @@ public class LockfileUtility { } + public static void waitForLockfileCreation(String path, String lockFileName) throws InterruptedException, IOException, TimeoutException { + if (Files.exists(Paths.get(path, lockFileName))) { + return; + } + try (WatchService watchService = FileSystems.getDefault().newWatchService()) { + Path dir = Paths.get(path); + dir.register(watchService, StandardWatchEventKinds.ENTRY_CREATE); + + WatchKey key = watchService.poll(10, TimeUnit.SECONDS); + if (key == null) { + throw new TimeoutException("Timeout waiting for lock file creation: " + lockFileName); + } + for (WatchEvent event : key.pollEvents()) { + WatchEvent.Kind kind = event.kind(); + if (kind == StandardWatchEventKinds.ENTRY_CREATE) { + Path createdFile = (Path) event.context(); + if (createdFile.toString().equals(lockFileName)) { + System.out.println("Lock file created: " + lockFileName); + return; + } + } + } + throw new TimeoutException("Timeout waiting for lock file creation: " + lockFileName); + } + } + + 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)); + try { + Process process = processBuilder.start(); + process.getErrorStream().transferTo(System.err); + process.getInputStream().transferTo(System.out); + process.waitFor(); + } catch (IOException | InterruptedException e) { + throw new RuntimeException(e); + } + }); + t.start(); + return t; + } + private static void lockWaitAndDelete(String path, String lockFileName, int seconds) throws InterruptedException, IOException { + File file = Paths.get(path,lockFileName).toFile(); + try (FileChannel channel = new RandomAccessFile(file.getAbsolutePath(), "rw").getChannel()) { FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false); if (fileLock != null) { - System.out.println("File "+lockFileName+" is locked. Waiting for " + seconds + " seconds to release"); + final ByteBuffer buff = ByteBuffer.wrap("Hello world".getBytes(StandardCharsets.UTF_8)); + channel.write(buff); + System.out.println("File "+lockFileName+" is locked. Waiting for " + seconds + " seconds to release. "); Thread.sleep(seconds * 1000L); fileLock.release(); + System.out.println(System.currentTimeMillis()); System.out.println("File "+lockFileName+" is released."); channel.close(); From 48066859d5b15e1b1d79e107b30dab37302ab04f Mon Sep 17 00:00:00 2001 From: dotasek Date: Wed, 18 Sep 2024 11:30:59 -0400 Subject: [PATCH 04/11] 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(); From 048aa2abe5cb4dc94938175835eae5c6ba3390b6 Mon Sep 17 00:00:00 2001 From: dotasek Date: Wed, 18 Sep 2024 16:07:00 -0400 Subject: [PATCH 05/11] More testing --- .../FilesystemPackageManagerLockTests.java | 19 +++++++++++++++ .../npm/FilesystemPackageManagerTests.java | 23 +++++++++++++++++++ 2 files changed, 42 insertions(+) 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 e20ecd747..34bde4ce8 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 @@ -1,6 +1,7 @@ package org.hl7.fhir.utilities.npm; import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -16,6 +17,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertTrue; import static org.junit.jupiter.api.Assertions.assertThrows; public class FilesystemPackageManagerLockTests { @@ -116,6 +118,23 @@ public class FilesystemPackageManagerLockTests { } } + @Test void testWhenLockIsntHeld_canLockFileBeHeldByThisProcessIsTrue() throws IOException { + File lockFile = getPackageLockFile(); + lockFile.createNewFile(); + Assertions.assertTrue(filesystemPackageCacheLockManager.getCacheLock().canLockFileBeHeldByThisProcess(lockFile)); + } + + @Test void testWhenLockIsHelp_canLockFileBeHeldByThisProcessIsFalse() throws IOException, InterruptedException, TimeoutException { + File lockFile = getPackageLockFile(); + Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(cachePath, DUMMY_PACKAGE + ".lock", 2); + + LockfileTestUtility.waitForLockfileCreation(cacheDirectory.getAbsolutePath(), DUMMY_PACKAGE + ".lock"); + + Assertions.assertFalse(filesystemPackageCacheLockManager.getCacheLock().canLockFileBeHeldByThisProcess(lockFile)); + + lockThread.join(); + } + @Test void testSinglePackageWriteMultiPackageRead() throws IOException { final FilesystemPackageCacheManagerLocks.PackageLock packageLock = filesystemPackageCacheLockManager.getPackageLock(DUMMY_PACKAGE); AtomicInteger writeCounter = new AtomicInteger(0); 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 d7565a348..dea38be40 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 @@ -134,6 +134,29 @@ public class FilesystemPackageManagerTests { assertThat(dummyLockFile).doesNotExist(); } + @Test + public void testLockedPackageIsntCleanedUp() throws IOException, InterruptedException, TimeoutException { + File cacheDirectory = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")); + + File dummyPackage = createDummyPackage(cacheDirectory, "example.fhir.uv.myig", "1.2.3"); + + Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock", 2); + + LockfileTestUtility.waitForLockfileCreation(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock"); + File dummyLockFile = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock"); + + assertThat(dummyPackage).isDirectory(); + assertThat(dummyPackage).exists(); + assertThat(dummyLockFile).exists(); + + FilesystemPackageCacheManager filesystemPackageCacheManager = new FilesystemPackageCacheManager.Builder().withCacheFolder(cacheDirectory.getAbsolutePath()).build(); + + assertThat(dummyPackage).exists(); + assertThat(dummyLockFile).exists(); + + lockThread.join(); + } + @Test public void testTimeoutForLockedPackageRead() throws IOException, InterruptedException, TimeoutException { String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath(); From b30134abfc6e3e3efa9ff69a68f6a7a339dca5a9 Mon Sep 17 00:00:00 2001 From: dotasek Date: Wed, 18 Sep 2024 18:40:13 -0400 Subject: [PATCH 06/11] WIP Switch to apache commons instead of nio for directory monitor --- .../npm/FilesystemPackageCacheManager.java | 3 + .../FilesystemPackageManagerLockTests.java | 24 ++- .../npm/FilesystemPackageManagerTests.java | 6 +- .../npm/LockfileTestProcessUtility.java | 117 +++++++++++++ .../utilities/npm/LockfileTestUtility.java | 155 ++++-------------- 5 files changed, 175 insertions(+), 130 deletions(-) create mode 100644 org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java 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 f3fba6551..206d90782 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 @@ -244,12 +244,15 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple if (file.getName().endsWith(".lock")) { if (locks.getCacheLock().canLockFileBeHeldByThisProcess(file)) { String packageDirectoryName = file.getName().substring(0, file.getName().length() - 5); + log("Detected potential incomplete package installed in cache: " + packageDirectoryName + ". Attempting to delete"); + File packageDirectory = ManagedFileAccess.file(Utilities.path(cacheFolder, packageDirectoryName)); if (packageDirectory.exists()) { Utilities.clearDirectory(packageDirectory.getAbsolutePath()); packageDirectory.delete(); } file.delete(); + log("Deleted potential incomplete package: " + packageDirectoryName); } } } 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 34bde4ce8..49eb750e8 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 @@ -124,9 +124,9 @@ public class FilesystemPackageManagerLockTests { Assertions.assertTrue(filesystemPackageCacheLockManager.getCacheLock().canLockFileBeHeldByThisProcess(lockFile)); } - @Test void testWhenLockIsHelp_canLockFileBeHeldByThisProcessIsFalse() throws IOException, InterruptedException, TimeoutException { + @Test void testWhenLockIsHelp_canLockFileBeHeldByThisProcessIsFalse() throws InterruptedException, TimeoutException, IOException { File lockFile = getPackageLockFile(); - Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(cachePath, DUMMY_PACKAGE + ".lock", 2); + Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cachePath, DUMMY_PACKAGE + ".lock", 2); LockfileTestUtility.waitForLockfileCreation(cacheDirectory.getAbsolutePath(), DUMMY_PACKAGE + ".lock"); @@ -198,11 +198,11 @@ public class FilesystemPackageManagerLockTests { } @Test - public void testReadWhenLockedByFileTimesOut() throws IOException, InterruptedException, TimeoutException { + public void testReadWhenLockedByFileTimesOut() throws InterruptedException, TimeoutException, IOException { FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager; final FilesystemPackageCacheManagerLocks.PackageLock packageLock = shorterTimeoutManager.getPackageLock(DUMMY_PACKAGE); File lockFile = getPackageLockFile(); - Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5); + Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5); LockfileTestUtility.waitForLockfileCreation(cachePath,lockFile.getName()); Exception exception = assertThrows(IOException.class, () -> { @@ -219,13 +219,25 @@ public class FilesystemPackageManagerLockTests { } @Test - public void testReadWhenLockFileIsDeleted() throws IOException, InterruptedException, TimeoutException { + public void apacheFileAlterationMonitorTest() { + + // Use Apache FileAlterationMonitor to monitor the cache directory for file deletions + // and create a lock file in a separate thread + + // Create a lock file in a separate thread + + + + } + + @Test + public void testReadWhenLockFileIsDeleted() throws InterruptedException, TimeoutException, IOException { final FilesystemPackageCacheManagerLocks.PackageLock packageLock = filesystemPackageCacheLockManager.getPackageLock(DUMMY_PACKAGE); final File lockFile = getPackageLockFile(); - Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5); + Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5); LockfileTestUtility.waitForLockfileCreation(cachePath,lockFile.getName()); packageLock.doReadWithLock(() -> { 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 dea38be40..1221aeb66 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 @@ -140,7 +140,7 @@ public class FilesystemPackageManagerTests { File dummyPackage = createDummyPackage(cacheDirectory, "example.fhir.uv.myig", "1.2.3"); - Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock", 2); + Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock", 2); LockfileTestUtility.waitForLockfileCreation(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock"); File dummyLockFile = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3.lock"); @@ -168,7 +168,7 @@ public class FilesystemPackageManagerTests { Assertions.assertTrue(pcm.listPackages().isEmpty()); - Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(pcmPath, "example.fhir.uv.myig#1.2.3.lock", 10); + Thread lockThread = LockfileTestProcessUtility.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(); @@ -198,7 +198,7 @@ public class FilesystemPackageManagerTests { File directory = ManagedFileAccess.file(pcmPath, packageAndVersion); directory.mkdir(); - Thread lockThread = LockfileTestUtility.lockWaitAndDeleteInNewProcess(pcmPath, "example.fhir.uv.myig#1.2.3.lock", 5); + Thread lockThread = LockfileTestProcessUtility.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"); diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java new file mode 100644 index 000000000..0e8f3f73d --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java @@ -0,0 +1,117 @@ +package org.hl7.fhir.utilities.npm; + +import java.io.File; +import java.io.IOException; +import java.io.RandomAccessFile; +import java.nio.ByteBuffer; +import java.nio.channels.FileChannel; +import java.nio.channels.FileLock; +import java.nio.charset.StandardCharsets; +import java.nio.file.Paths; + +/** + * 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 LockfileTestProcessUtility { + /** + * Main method to allow running this class. + *

+ * 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:.", LockfileTestProcessUtility.class.getName(), path, lockFileName, Integer.toString(seconds)); + try { + Process process = processBuilder.start(); + process.getErrorStream().transferTo(System.err); + process.getInputStream().transferTo(System.out); + process.waitFor(); + } catch (IOException | InterruptedException e) { + throw new RuntimeException(e); + } + }); + t.start(); + 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(); + + try (FileChannel channel = new RandomAccessFile(file.getAbsolutePath(), "rw").getChannel()) { + FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false); + if (fileLock != null) { + final ByteBuffer buff = ByteBuffer.wrap("Hello world".getBytes(StandardCharsets.UTF_8)); + channel.write(buff); + System.out.println("File "+lockFileName+" is locked. Waiting for " + seconds + " seconds to release. "); + Thread.sleep(seconds * 1000L); + file.delete(); + fileLock.release(); + System.out.println(System.currentTimeMillis()); + System.out.println("File "+lockFileName+" is released."); + + channel.close(); + }}finally { + if (file.exists()) { + file.delete(); + } + } + } +} diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java index 9ea70fda0..44949ac6e 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java @@ -1,52 +1,21 @@ package org.hl7.fhir.utilities.npm; +import org.apache.commons.io.monitor.FileAlterationListenerAdaptor; +import org.apache.commons.io.monitor.FileAlterationMonitor; +import org.apache.commons.io.monitor.FileAlterationObserver; + import java.io.*; -import java.nio.ByteBuffer; -import java.nio.channels.FileChannel; -import java.nio.channels.FileLock; -import java.nio.charset.StandardCharsets; + import java.nio.file.*; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; -/** - * 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]; - int seconds = Integer.parseInt(args[2]); - try { - lockWaitAndDelete(path, lockFileName, seconds); - } catch (InterruptedException | IOException e) { - throw new RuntimeException(e); - } - - } /** * Wait for the lock file to be created in the given path. @@ -57,98 +26,42 @@ public class LockfileTestUtility { * @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 { + public static void waitForLockfileCreation(String path, String lockFileName) throws InterruptedException, TimeoutException { if (Files.exists(Paths.get(path, lockFileName))) { return; } - try (WatchService watchService = FileSystems.getDefault().newWatchService()) { - Path dir = Paths.get(path); - dir.register(watchService, StandardWatchEventKinds.ENTRY_CREATE); + CountDownLatch latch = new CountDownLatch(1); + FileAlterationMonitor monitor = new FileAlterationMonitor(100); + FileAlterationObserver observer = new FileAlterationObserver(path); - WatchKey key = watchService.poll(10, TimeUnit.SECONDS); - if (key == null) { - throw new TimeoutException("Timeout waiting for lock file creation: " + lockFileName); - } - for (WatchEvent event : key.pollEvents()) { - WatchEvent.Kind kind = event.kind(); - if (kind == StandardWatchEventKinds.ENTRY_CREATE) { - Path createdFile = (Path) event.context(); - if (createdFile.toString().equals(lockFileName)) { - System.out.println("Lock file created: " + lockFileName); - return; - } - } - } - throw new TimeoutException("Timeout waiting for lock file creation: " + lockFileName); - } - } - - /** - * 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:.", LockfileTestUtility.class.getName(), path, lockFileName, Integer.toString(seconds)); - try { - Process process = processBuilder.start(); - process.getErrorStream().transferTo(System.err); - process.getInputStream().transferTo(System.out); - process.waitFor(); - } catch (IOException | InterruptedException e) { - throw new RuntimeException(e); + observer.addListener(new FileAlterationListenerAdaptor(){ + @Override + public void onFileCreate(File file) { + System.out.println("File created: " + file.getName()); + latch.countDown(); } }); - t.start(); - return t; - } + monitor.addObserver(observer); - /** - * 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(); - - try (FileChannel channel = new RandomAccessFile(file.getAbsolutePath(), "rw").getChannel()) { - FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false); - if (fileLock != null) { - final ByteBuffer buff = ByteBuffer.wrap("Hello world".getBytes(StandardCharsets.UTF_8)); - channel.write(buff); - System.out.println("File "+lockFileName+" is locked. Waiting for " + seconds + " seconds to release. "); - Thread.sleep(seconds * 1000L); - fileLock.release(); - System.out.println(System.currentTimeMillis()); - System.out.println("File "+lockFileName+" is released."); - - channel.close(); - }}finally { - if (file.exists()) { - file.delete(); - } + try { + monitor.start(); + } catch (Exception e) { + throw new RuntimeException(e); } + latch.await(10, TimeUnit.SECONDS); + try { + monitor.stop(); + } catch (Exception e) { + throw new RuntimeException(e); + } + + if (!Files.exists(Paths.get(path, lockFileName))) { + throw new TimeoutException("Lock file not created within 10 seconds"); + } + } + + } From 0fd25a6a986542130b08b4ead50db4fd90b3109d Mon Sep 17 00:00:00 2001 From: dotasek Date: Wed, 18 Sep 2024 19:23:00 -0400 Subject: [PATCH 07/11] Move file exists logic into onStart + move file delete before unlock --- .../FilesystemPackageCacheManagerLocks.java | 5 ++- .../FilesystemPackageManagerLockTests.java | 11 ----- .../utilities/npm/LockfileTestUtility.java | 40 +++++++++++-------- 3 files changed, 26 insertions(+), 30 deletions(-) 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 582596420..ad9d54380 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 @@ -261,11 +261,12 @@ public class FilesystemPackageCacheManagerLocks { try { result = function.get(); } finally { - fileLock.release(); - channel.close(); if (!lockFile.delete()) { lockFile.deleteOnExit(); } + fileLock.release(); + channel.close(); + lock.writeLock().unlock(); cacheLock.getLock().writeLock().unlock(); } 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 49eb750e8..8fa981d88 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 @@ -218,17 +218,6 @@ public class FilesystemPackageManagerLockTests { lockThread.join(); } - @Test - public void apacheFileAlterationMonitorTest() { - - // Use Apache FileAlterationMonitor to monitor the cache directory for file deletions - // and create a lock file in a separate thread - - // Create a lock file in a separate thread - - - - } @Test public void testReadWhenLockFileIsDeleted() throws InterruptedException, TimeoutException, IOException { diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java index 44949ac6e..bd2c2c4e4 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java @@ -29,14 +29,20 @@ public class LockfileTestUtility { * @throws TimeoutException If the lock file is not created within 10 seconds */ public static void waitForLockfileCreation(String path, String lockFileName) throws InterruptedException, TimeoutException { - if (Files.exists(Paths.get(path, lockFileName))) { - return; - } + CountDownLatch latch = new CountDownLatch(1); FileAlterationMonitor monitor = new FileAlterationMonitor(100); FileAlterationObserver observer = new FileAlterationObserver(path); observer.addListener(new FileAlterationListenerAdaptor(){ + + @Override + public void onStart(FileAlterationObserver observer) { + if (Files.exists(Paths.get(path, lockFileName))) { + latch.countDown(); + } + } + @Override public void onFileCreate(File file) { System.out.println("File created: " + file.getName()); @@ -45,21 +51,21 @@ public class LockfileTestUtility { }); monitor.addObserver(observer); - try { - monitor.start(); - } catch (Exception e) { - throw new RuntimeException(e); - } - latch.await(10, TimeUnit.SECONDS); - try { - monitor.stop(); - } catch (Exception e) { - throw new RuntimeException(e); - } + try { + monitor.start(); + } catch (Exception e) { + throw new RuntimeException(e); + } + boolean success = latch.await(10, TimeUnit.SECONDS); + try { + monitor.stop(); + } catch (Exception e) { + throw new RuntimeException(e); + } - if (!Files.exists(Paths.get(path, lockFileName))) { - throw new TimeoutException("Lock file not created within 10 seconds"); - } + if (!success) { + throw new TimeoutException("Timed out waiting for lock file creation: " + lockFileName); + } } From cce4d108ceeed22296f085d526ea49700e0a3eb8 Mon Sep 17 00:00:00 2001 From: dotasek Date: Thu, 19 Sep 2024 10:48:30 -0400 Subject: [PATCH 08/11] Fix classpath on windows + workaround for edge case on lock file --- .../hl7/fhir/utilities/npm/LockfileTestProcessUtility.java | 2 +- .../java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java index 0e8f3f73d..76792c5fb 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java @@ -62,7 +62,7 @@ public class LockfileTestProcessUtility { */ public static Thread lockWaitAndDeleteInNewProcess(String path, String lockFileName, int seconds) { Thread t = new Thread(() -> { - ProcessBuilder processBuilder = new ProcessBuilder("java", "-cp", "target/test-classes:.", LockfileTestProcessUtility.class.getName(), path, lockFileName, Integer.toString(seconds)); + ProcessBuilder processBuilder = new ProcessBuilder("java", "-cp", "target/test-classes", LockfileTestProcessUtility.class.getName(), path, lockFileName, Integer.toString(seconds)); try { Process process = processBuilder.start(); process.getErrorStream().transferTo(System.err); diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java index bd2c2c4e4..e7badf2f0 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java @@ -66,6 +66,11 @@ public class LockfileTestUtility { if (!success) { throw new TimeoutException("Timed out waiting for lock file creation: " + lockFileName); } + // TODO This is a workaround for an edge condition that shows up with testing, where the lock is not reflected in + // the file system immediately. It is unlikely to appear in production environments, but should it occur, it will + // result in a lock file being erroneously reported as not having an owning process, and will cause a package to + // fail to be loaded from that cache until the lock is cleaned up by cache initialization. + Thread.sleep(100); } From c81c631ae8c5c479e366b365326714bc85a95333 Mon Sep 17 00:00:00 2001 From: dotasek Date: Thu, 19 Sep 2024 12:22:29 -0400 Subject: [PATCH 09/11] Use rename to bypass windows lockfile issue --- .../npm/FilesystemPackageCacheManagerLocks.java | 8 ++++++-- .../utilities/npm/LockfileTestProcessUtility.java | 15 +++++++++------ .../fhir/utilities/npm/LockfileTestUtility.java | 2 +- 3 files changed, 16 insertions(+), 9 deletions(-) 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 ad9d54380..def6e8cb7 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 @@ -261,11 +261,15 @@ public class FilesystemPackageCacheManagerLocks { try { result = function.get(); } finally { + + lockFile.renameTo(File.createTempFile(lockFile.getName(), ".lock-renamed")); + + fileLock.release(); + channel.close(); + if (!lockFile.delete()) { lockFile.deleteOnExit(); } - fileLock.release(); - channel.close(); lock.writeLock().unlock(); cacheLock.getLock().writeLock().unlock(); diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java index 76792c5fb..63e10bfd9 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java @@ -93,24 +93,27 @@ public class LockfileTestProcessUtility { */ private static void lockWaitAndDelete(String path, String lockFileName, int seconds) throws InterruptedException, IOException { - File file = Paths.get(path,lockFileName).toFile(); + File lockFile = Paths.get(path,lockFileName).toFile(); - try (FileChannel channel = new RandomAccessFile(file.getAbsolutePath(), "rw").getChannel()) { + try (FileChannel channel = new RandomAccessFile(lockFile.getAbsolutePath(), "rw").getChannel()) { FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false); if (fileLock != null) { final ByteBuffer buff = ByteBuffer.wrap("Hello world".getBytes(StandardCharsets.UTF_8)); channel.write(buff); System.out.println("File "+lockFileName+" is locked. Waiting for " + seconds + " seconds to release. "); Thread.sleep(seconds * 1000L); - file.delete(); + + lockFile.renameTo(File.createTempFile(lockFile.getName(), ".lock-renamed")); + fileLock.release(); + channel.close(); System.out.println(System.currentTimeMillis()); System.out.println("File "+lockFileName+" is released."); - channel.close(); + lockFile.delete(); }}finally { - if (file.exists()) { - file.delete(); + if (lockFile.exists()) { + lockFile.delete(); } } } diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java index e7badf2f0..5309d5e19 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java @@ -70,7 +70,7 @@ public class LockfileTestUtility { // the file system immediately. It is unlikely to appear in production environments, but should it occur, it will // result in a lock file being erroneously reported as not having an owning process, and will cause a package to // fail to be loaded from that cache until the lock is cleaned up by cache initialization. - Thread.sleep(100); + //Thread.sleep(100); } From 356a70a8454f28c69cef4560f4953ffb16cd81d8 Mon Sep 17 00:00:00 2001 From: dotasek Date: Thu, 19 Sep 2024 12:53:17 -0400 Subject: [PATCH 10/11] Re-introduce sleep to lockfile creation wait. --- .../java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java index 5309d5e19..e7badf2f0 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java @@ -70,7 +70,7 @@ public class LockfileTestUtility { // the file system immediately. It is unlikely to appear in production environments, but should it occur, it will // result in a lock file being erroneously reported as not having an owning process, and will cause a package to // fail to be loaded from that cache until the lock is cleaned up by cache initialization. - //Thread.sleep(100); + Thread.sleep(100); } From d57008510c8ca3bd821849d51f4132d7beeaaf12 Mon Sep 17 00:00:00 2001 From: dotasek Date: Thu, 19 Sep 2024 13:31:38 -0400 Subject: [PATCH 11/11] Use ManagedFileAccess for temp file creation --- .../utilities/npm/FilesystemPackageCacheManagerLocks.java | 3 ++- .../hl7/fhir/utilities/npm/LockfileTestProcessUtility.java | 4 +++- .../java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) 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 def6e8cb7..b32dda96d 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 @@ -3,6 +3,7 @@ package org.hl7.fhir.utilities.npm; import lombok.Getter; import lombok.With; import org.hl7.fhir.utilities.Utilities; +import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -262,7 +263,7 @@ public class FilesystemPackageCacheManagerLocks { result = function.get(); } finally { - lockFile.renameTo(File.createTempFile(lockFile.getName(), ".lock-renamed")); + lockFile.renameTo(ManagedFileAccess.file(File.createTempFile(lockFile.getName(), ".lock-renamed").getAbsolutePath())); fileLock.release(); channel.close(); diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java index 63e10bfd9..d8f054f3d 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java @@ -1,5 +1,7 @@ package org.hl7.fhir.utilities.npm; +import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; + import java.io.File; import java.io.IOException; import java.io.RandomAccessFile; @@ -103,7 +105,7 @@ public class LockfileTestProcessUtility { System.out.println("File "+lockFileName+" is locked. Waiting for " + seconds + " seconds to release. "); Thread.sleep(seconds * 1000L); - lockFile.renameTo(File.createTempFile(lockFile.getName(), ".lock-renamed")); + lockFile.renameTo(ManagedFileAccess.file(File.createTempFile(lockFile.getName(), ".lock-renamed").getAbsolutePath())); fileLock.release(); channel.close(); diff --git a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java index e7badf2f0..250e7bcb1 100644 --- a/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java @@ -67,7 +67,7 @@ public class LockfileTestUtility { throw new TimeoutException("Timed out waiting for lock file creation: " + lockFileName); } // TODO This is a workaround for an edge condition that shows up with testing, where the lock is not reflected in - // the file system immediately. It is unlikely to appear in production environments, but should it occur, it will + // the file system immediately. It is unlikely to appear in production environments. Should it occur, it will // result in a lock file being erroneously reported as not having an owning process, and will cause a package to // fail to be loaded from that cache until the lock is cleaned up by cache initialization. Thread.sleep(100);