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 ec04958745..206d90782a 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.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 interface IPackageProvider { 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 interface IPackageProvider { private final Map ciList = new HashMap<>(); private JsonArray buildInfo; private boolean suppressErrors; + @Setter @Getter private boolean minimalMemory; @@ -113,9 +117,20 @@ public static class Builder { @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 @@ protected List getConfiguredServers() { 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,32 +173,33 @@ public Builder withSystemCacheFolder() throws IOException { } 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 { - return new FilesystemPackageCacheManager(cacheFolder, packageServers); + final FilesystemPackageCacheManagerLocks locks; + try { + locks = FilesystemPackageCacheManagerLocks.getFilesystemPackageCacheManagerLocks(cacheFolder); + } catch (RuntimeException e) { + if (e.getCause() instanceof IOException) { + throw (IOException) e.getCause(); + } else { + throw e; + } + } + return new FilesystemPackageCacheManager(cacheFolder, packageServers, locks, lockParameters); } } - private FilesystemPackageCacheManager(@Nonnull File cacheFolder, @Nonnull List packageServers) throws IOException { + private FilesystemPackageCacheManager(@Nonnull File cacheFolder, @Nonnull List packageServers, @Nonnull FilesystemPackageCacheManagerLocks locks, @Nullable FilesystemPackageCacheManagerLocks.LockParameters lockParameters) throws IOException { super(packageServers); this.cacheFolder = cacheFolder; - - try { - this.locks = FilesystemPackageCacheManagerLocks.getFilesystemPackageCacheManagerLocks(cacheFolder); - } catch (RuntimeException e) { - if (e.getCause() instanceof IOException) { - throw (IOException) e.getCause(); - } else { - throw e; - } - } - + this.locks = locks; + this.lockParameters = lockParameters; prepareCacheFolder(); } @@ -218,11 +229,35 @@ protected void prepareCacheFolder() throws IOException { createIniFile(); } 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")) { + 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); + } + } + } + } + private boolean iniFileExists() throws IOException { String iniPath = getPackagesIniPath(); File iniFile = ManagedFileAccess.file(iniPath); @@ -421,7 +456,7 @@ public void removePackage(String id, String version) throws IOException { } return null; - }); + }, lockParameters); } /** @@ -465,7 +500,7 @@ public NpmPackage loadPackageFromCacheOnly(String id, String version) throws IOE return null; } return loadPackageInfo(path); - }); + }, lockParameters); if (foundPackage != null) { if (foundPackage.isIndexed()){ return foundPackage; @@ -488,7 +523,7 @@ public NpmPackage loadPackageFromCacheOnly(String id, String version) throws IOE String path = Utilities.path(cacheFolder, foundPackageFolder); output.checkIndexed(path); return output; - }); + }, lockParameters); } } } @@ -589,7 +624,7 @@ public NpmPackage addPackageToCache(final String id, final String version, final 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 d674c750e4..b32dda96dc 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,19 @@ 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 org.hl7.fhir.utilities.filesystem.ManagedFileAccess; +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 +32,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 +62,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. @@ -102,6 +109,19 @@ public T doWriteWithLock(FilesystemPackageCacheManager.CacheLockFunction } 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 { @@ -114,15 +134,43 @@ protected PackageLock(File lockFile, ReadWriteLock lock) { this.lock = lock; } - private void checkForLockFileWaitForDeleteIfExists(File lockFile) throws IOException { + private void checkForLockFileWaitForDeleteIfExists(File lockFile, @Nonnull LockParameters lockParameters) throws IOException { 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, false); + if (fileLock != null) { + fileLock.release(); + 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, lockParameters); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Thread interrupted while waiting for lock", e); + } + } + + /* + 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, @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. @@ -141,24 +189,33 @@ private void checkForLockFileWaitForDeleteIfExists(File lockFile) throws IOExcep key.reset(); } } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new IOException("Error reading package.", e); } catch (TimeoutException e) { - throw new IOException("Error reading package.", e); + throw new IOException("Package cache timed out waiting for lock.", e); } } - 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(); @@ -166,35 +223,55 @@ public T doReadWithLock(FilesystemPackageCacheManager.CacheLockFunction f 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(); - if (!lockFile.isFile()) { - try { - TextFile.stringToFile("", lockFile); - } catch (IOException e) { - e.printStackTrace(); - return null; - } - } + /*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 = null; - while (fileLock == null) { - fileLock = channel.tryLock(0, Long.MAX_VALUE, true); - if (fileLock == null) { - Thread.sleep(100); // Wait and retry - } + + FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false); + + if (fileLock == null) { + 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()) { + 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 { + + lockFile.renameTo(ManagedFileAccess.file(File.createTempFile(lockFile.getName(), ".lock-renamed").getAbsolutePath())); + fileLock.release(); channel.close(); + if (!lockFile.delete()) { lockFile.deleteOnExit(); } + 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 f1d71dda3e..8fa981d885 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,7 +1,7 @@ package org.hl7.fhir.utilities.npm; -import org.hl7.fhir.utilities.TextFile; 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; @@ -12,10 +12,12 @@ 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; 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 { @@ -49,13 +51,13 @@ public void testBaseCases() throws IOException { 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 +89,7 @@ public void testBaseCases() throws IOException { packageLock.doWriteWithLock(() -> { assertThat(cacheLockFinished.get()).isTrue(); return null; - }); + }, null); } catch (IOException e) { throw new RuntimeException(e); } @@ -97,7 +99,7 @@ public void testBaseCases() throws IOException { packageLock.doReadWithLock(() -> { assertThat(cacheLockFinished.get()).isTrue(); return null; - }); + }, null); } catch (IOException e) { throw new RuntimeException(e); } @@ -116,6 +118,23 @@ public void testBaseCases() throws IOException { } } + @Test void testWhenLockIsntHeld_canLockFileBeHeldByThisProcessIsTrue() throws IOException { + File lockFile = getPackageLockFile(); + lockFile.createNewFile(); + Assertions.assertTrue(filesystemPackageCacheLockManager.getCacheLock().canLockFileBeHeldByThisProcess(lockFile)); + } + + @Test void testWhenLockIsHelp_canLockFileBeHeldByThisProcessIsFalse() throws InterruptedException, TimeoutException, IOException { + File lockFile = getPackageLockFile(); + Thread lockThread = LockfileTestProcessUtility.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); @@ -133,7 +152,7 @@ public void testBaseCases() throws IOException { assertThat(writeCount).isEqualTo(1); writeCounter.decrementAndGet(); return null; - }); + }, null); } catch (IOException e) { throw new RuntimeException(e); } @@ -156,7 +175,7 @@ public void testBaseCases() throws IOException { } readCounter.decrementAndGet(); return null; - }); + }, null); } catch (IOException e) { throw new RuntimeException(e); } @@ -179,49 +198,47 @@ public void testBaseCases() throws IOException { } @Test - public void testReadWhenLockedByFileTimesOut() throws IOException { - FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager.withLockTimeout(3L, TimeUnit.SECONDS); + public void testReadWhenLockedByFileTimesOut() throws InterruptedException, TimeoutException, IOException { + FilesystemPackageCacheManagerLocks shorterTimeoutManager = filesystemPackageCacheLockManager; final FilesystemPackageCacheManagerLocks.PackageLock packageLock = shorterTimeoutManager.getPackageLock(DUMMY_PACKAGE); - File lockFile = createPackageLockFile(); + File lockFile = getPackageLockFile(); + Thread lockThread = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5); + LockfileTestUtility.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 InterruptedException, TimeoutException, IOException { - 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 = LockfileTestProcessUtility.lockWaitAndDeleteInNewProcess(cachePath, lockFile.getName(), 5); + LockfileTestUtility.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 eba7b198a9..1221aeb66d 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 @@ -2,6 +2,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.io.File; @@ -13,6 +14,8 @@ import java.util.Random; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Stream; @@ -20,6 +23,7 @@ import org.hl7.fhir.utilities.IniFile; import org.hl7.fhir.utilities.filesystem.ManagedFileAccess; +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; @@ -113,6 +117,97 @@ public void testSystemCacheDirectoryWin() throws IOException { 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 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 = 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"); + + 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(); + + final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder() + .withCacheFolder(pcmPath) + .withLockParameters(new FilesystemPackageCacheManagerLocks.LockParameters(5,TimeUnit.SECONDS)) + .build(); + + Assertions.assertTrue(pcm.listPackages().isEmpty()); + + 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(); + + 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")); + + assertThat(exception.getMessage()).contains("Package cache timed out waiting for lock"); + assertThat(exception.getCause().getMessage()).contains("Timeout waiting for lock file deletion"); + lockThread.join(); + } + + @Test + 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(); + + Assertions.assertTrue(pcm.listPackages().isEmpty()); + + 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"); + + String packageAndVersion = "example.fhir.uv.myig#1.2.3"; + + //Now sneak in a new lock file and directory: + + File directory = ManagedFileAccess.file(pcmPath, packageAndVersion); + directory.mkdir(); + + 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"); + + assertThat(npmPackage.id()).isEqualTo("example.fhir.uv.myig"); + + lockThread.join(); + } + /** We repeat the same tests multiple times here, in order to catch very rare edge cases. */ @@ -126,16 +221,16 @@ public static Stream packageCacheMultiThreadTestParams() { 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(); @@ -144,6 +239,16 @@ private static void createDummyPackage(File cacheDirectory, String directoryName 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 { @@ -241,13 +346,13 @@ private void assertInitializedTestCacheIsValid(File cacheDirectory, boolean dumm @MethodSource("packageCacheMultiThreadTestParams") @ParameterizedTest public void packageCacheMultiThreadTest(final int threadTotal, final int packageCacheManagerTotal) throws IOException { - String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath(); + System.out.println("Using temp pcm path: " + pcmPath); FilesystemPackageCacheManager[] packageCacheManagers = new FilesystemPackageCacheManager[packageCacheManagerTotal]; 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; @@ -256,22 +361,27 @@ public void packageCacheMultiThreadTest(final int threadTotal, final int package 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"); 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 0000000000..d8f054f3d7 --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestProcessUtility.java @@ -0,0 +1,122 @@ +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; +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 lockFile = Paths.get(path,lockFileName).toFile(); + + 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); + + lockFile.renameTo(ManagedFileAccess.file(File.createTempFile(lockFile.getName(), ".lock-renamed").getAbsolutePath())); + + fileLock.release(); + channel.close(); + System.out.println(System.currentTimeMillis()); + System.out.println("File "+lockFileName+" is released."); + + lockFile.delete(); + }}finally { + 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 new file mode 100644 index 0000000000..250e7bcb17 --- /dev/null +++ b/org.hl7.fhir.utilities/src/test/java/org/hl7/fhir/utilities/npm/LockfileTestUtility.java @@ -0,0 +1,78 @@ +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.file.*; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + + +public class LockfileTestUtility { + + + + /** + * 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 TimeoutException If the lock file is not created within 10 seconds + */ + public static void waitForLockfileCreation(String path, String lockFileName) throws InterruptedException, TimeoutException { + + 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()); + latch.countDown(); + } + }); + monitor.addObserver(observer); + + 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 (!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. 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); + + } + + +}