WIP master merges + smarter lockfile tests

RELEASE_NOTES.md

@@ -1,18 +1,7 @@
 ## Validator Changes
 
-* Fix expression for con-3 properly (fix validation problem on some condition resources)
+* no changes
-* Fix FHIRPath bug using wrong type on simple elements when checking FHIRPath types
-* FHIRPath: Allow _ in constant names (per FHIRPath spec)
-* Fix value set rendering creating wrong references
-* Fix bug processing value set includes / excludes that are just value sets (no system value)
-* Alter processing of unknown code systems per discussion at ,https://chat.fhir.org/#narrow/stream/179252-IG-creation/topic/Don't.20error.20when.20you.20can't.20find.20code.20system and implement unknown-codesystems-cause-errors
-* Improve message for when elements are out of order in profile differentials
-
 
 ## Other code changes
 
-* fix problem where profile rendering had spurious 'slices for' nodes everywhere
+* no changes
-* Update SQL-On-FHIR implementation for latest cases, and clone test cases to general test care repository
-* Fix problem generating value set spreadsheets
-* fix concurrent modification error processing language translations
-* Check for null fetcher processing ConceptMaps (#1728)

org.hl7.fhir.convertors/pom.xml

@@ -5,7 +5,7 @@
     <parent>
         <groupId>ca.uhn.hapi.fhir</groupId>
         <artifactId>org.hl7.fhir.core</artifactId>
-        <version>6.3.24-SNAPSHOT</version>
+        <version>6.3.26-SNAPSHOT</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

org.hl7.fhir.dstu2/pom.xml

@@ -5,7 +5,7 @@
 	<parent>
 		<groupId>ca.uhn.hapi.fhir</groupId>
 		<artifactId>org.hl7.fhir.core</artifactId>
-		<version>6.3.24-SNAPSHOT</version>
+		<version>6.3.26-SNAPSHOT</version>
 		<relativePath>../pom.xml</relativePath>
 	</parent>
 

org.hl7.fhir.dstu2016may/pom.xml

@@ -5,7 +5,7 @@
 	<parent>
 		<groupId>ca.uhn.hapi.fhir</groupId>
 		<artifactId>org.hl7.fhir.core</artifactId>
-		<version>6.3.24-SNAPSHOT</version>
+		<version>6.3.26-SNAPSHOT</version>
 		<relativePath>../pom.xml</relativePath>
 	</parent>
 

org.hl7.fhir.dstu3/pom.xml

@@ -5,7 +5,7 @@
     <parent>
         <groupId>ca.uhn.hapi.fhir</groupId>
         <artifactId>org.hl7.fhir.core</artifactId>
-        <version>6.3.24-SNAPSHOT</version>
+        <version>6.3.26-SNAPSHOT</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

org.hl7.fhir.r4/pom.xml

@@ -5,7 +5,7 @@
 	<parent>
 		<groupId>ca.uhn.hapi.fhir</groupId>
 		<artifactId>org.hl7.fhir.core</artifactId>
-		<version>6.3.24-SNAPSHOT</version>
+		<version>6.3.26-SNAPSHOT</version>
 		<relativePath>../pom.xml</relativePath>
 	</parent>
 

org.hl7.fhir.r4b/pom.xml

@@ -5,7 +5,7 @@
 	<parent>
 		<groupId>ca.uhn.hapi.fhir</groupId>
 		<artifactId>org.hl7.fhir.core</artifactId>
-		<version>6.3.24-SNAPSHOT</version>
+		<version>6.3.26-SNAPSHOT</version>
 		<relativePath>../pom.xml</relativePath>
 	</parent>
 

org.hl7.fhir.r5/pom.xml

@@ -5,7 +5,7 @@
 	<parent>
 		<groupId>ca.uhn.hapi.fhir</groupId>
 		<artifactId>org.hl7.fhir.core</artifactId>
-		<version>6.3.24-SNAPSHOT</version>
+		<version>6.3.26-SNAPSHOT</version>
 		<relativePath>../pom.xml</relativePath>
 	</parent>
 

org.hl7.fhir.report/pom.xml

@@ -5,7 +5,7 @@
     <parent>
         <groupId>ca.uhn.hapi.fhir</groupId>
         <artifactId>org.hl7.fhir.core</artifactId>
-        <version>6.3.24-SNAPSHOT</version>
+        <version>6.3.26-SNAPSHOT</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

org.hl7.fhir.utilities/pom.xml

@@ -5,7 +5,7 @@
     <parent>
         <groupId>ca.uhn.hapi.fhir</groupId>
         <artifactId>org.hl7.fhir.core</artifactId>
-        <version>6.3.24-SNAPSHOT</version>
+        <version>6.3.26-SNAPSHOT</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

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

@@ -171,16 +171,9 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
     }
 
     public FilesystemPackageCacheManager build() throws IOException {
-      return new FilesystemPackageCacheManager(cacheFolder, packageServers);
+      final FilesystemPackageCacheManagerLocks locks;
-    }
-  }
-
-  private FilesystemPackageCacheManager(@Nonnull File cacheFolder, @Nonnull List<PackageServer> packageServers) throws IOException {
-    super(packageServers);
-    this.cacheFolder = cacheFolder;
-
       try {
-      this.locks = FilesystemPackageCacheManagerLocks.getFilesystemPackageCacheManagerLocks(cacheFolder);
+        locks = FilesystemPackageCacheManagerLocks.getFilesystemPackageCacheManagerLocks(cacheFolder);
       } catch (RuntimeException e) {
         if (e.getCause() instanceof IOException) {
           throw (IOException) e.getCause();
@@ -188,6 +181,14 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
           throw e;
         }
       }
+      return new FilesystemPackageCacheManager(cacheFolder, packageServers, locks);
+    }
+  }
+
+  private FilesystemPackageCacheManager(@Nonnull File cacheFolder, @Nonnull List<PackageServer> packageServers, @Nonnull FilesystemPackageCacheManagerLocks locks) throws IOException {
+    super(packageServers);
+    this.cacheFolder = cacheFolder;
+    this.locks = locks;
 
     prepareCacheFolder();
   }
@@ -210,14 +211,16 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
         Utilities.createDirectory(cacheFolder.getAbsolutePath());
         createIniFile();
       } else {
-        if (!isCacheFolderValid()) {
+        if (!iniFileExists()) {
+          createIniFile();
+        }
+        if (!isIniFileCurrentVersion()) {
           clearCache();
           createIniFile();
-        } else {
+        }
         deleteOldTempDirectories();
         cleanUpCorruptPackages();
       }
-      }
       return null;
     });
   }
@@ -240,15 +243,17 @@ public class FilesystemPackageCacheManager extends BasePackageCacheManager imple
     }
   }
 
-  private boolean isCacheFolderValid() throws IOException {
+  private boolean iniFileExists() throws IOException {
     String iniPath = getPackagesIniPath();
     File iniFile = ManagedFileAccess.file(iniPath);
-    if (!(iniFile.exists())) {
+    return iniFile.exists();
-      return false;
   }
+
+  private boolean isIniFileCurrentVersion() throws IOException {
+    String iniPath = getPackagesIniPath();
     IniFile ini = new IniFile(iniPath);
-    String v = ini.getStringProperty("cache", "version");
+    String version = ini.getStringProperty("cache", "version");
-    return CACHE_VERSION.equals(v);
+    return CACHE_VERSION.equals(version);
   }
 
   private void deleteOldTempDirectories() throws IOException {

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

@@ -123,7 +123,7 @@ public class FilesystemPackageCacheManagerLocks {
       // 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);
+          FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
           if (fileLock != null) {
             fileLock.release();
             channel.close();
@@ -131,7 +131,19 @@ public class FilesystemPackageCacheManagerLocks {
           }
         }
       }
+      try {
+        waitForLockFileDeletion(lockFile);
+      } 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) throws IOException, InterruptedException {
       try (WatchService watchService = FileSystems.getDefault().newWatchService()) {
         Path dir = lockFile.getParentFile().toPath();
         dir.register(watchService, StandardWatchEventKinds.ENTRY_DELETE);
@@ -155,11 +167,8 @@ public class FilesystemPackageCacheManagerLocks {
             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);
       }
     }
 
@@ -184,20 +193,19 @@ public class FilesystemPackageCacheManagerLocks {
       cacheLock.getLock().writeLock().lock();
       lock.writeLock().lock();
 
+      try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) {
+
+        FileLock fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
+
+        if (fileLock == null) {
+          waitForLockFileDeletion(lockFile);
+        }
+
         if (!lockFile.isFile()) {
           try {
-          TextFile.stringToFile("", lockFile);
+            TextFile.stringToFile(String.valueOf(ProcessHandle.current().pid()), lockFile);
           } catch (IOException e) {
-          e.printStackTrace();
+            throw new IOException("Error writing lock file.", e);
-          return null;
-        }
-      }
-      try (FileChannel channel = new RandomAccessFile(lockFile, "rw").getChannel()) {
-        FileLock fileLock = null;
-        while (fileLock == null) {
-          fileLock = channel.tryLock(0, Long.MAX_VALUE, false);
-          if (fileLock == null) {
-            Thread.sleep(100); // Wait and retry
           }
         }
         T result = null;

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

@@ -648,7 +648,17 @@ public class NpmPackage {
   }
 
 
-
+  /**
+   * Create a package .index.json file for a package folder.
+   * <p>
+   * See <a href="https://hl7.org/fhir/packages.html#2.1.10.4">the FHIR specification</a> for details on .index.json
+   * format and usage.
+   *
+   * @param desc
+   * @param folder
+   * @throws FileNotFoundException
+   * @throws IOException
+   */
   public void indexFolder(String desc, NpmPackageFolder folder) throws FileNotFoundException, IOException {
     List<String> remove = new ArrayList<>();
     NpmPackageIndexBuilder indexer = new NpmPackageIndexBuilder();

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

@@ -1,28 +1,27 @@
 package org.hl7.fhir.utilities.npm;
 
-import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.io.File;
 
+import java.io.FileWriter;
 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.UUID;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
-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.IniFile;
 import org.hl7.fhir.utilities.filesystem.ManagedFileAccess;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -108,7 +107,6 @@ public class FilesystemPackageManagerTests {
   @DisabledOnOs(OS.WINDOWS)
   public void testSystemCacheDirectory() throws IOException {
     File folder = new FilesystemPackageCacheManager.Builder().withSystemCacheFolder().getCacheFolder();
-
     assertEquals( "/var/lib/.fhir/packages", folder.getAbsolutePath());
   }
 
@@ -120,7 +118,7 @@ public class FilesystemPackageManagerTests {
   }
 
   @Test
-  public void testFailureForUnlockedLockFiles() throws IOException, InterruptedException {
+  public void testTimeoutForLockedPackageRead() throws IOException, InterruptedException {
     String pcmPath = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest")).getAbsolutePath();
 
     final FilesystemPackageCacheManager pcm = new FilesystemPackageCacheManager.Builder().withCacheFolder(pcmPath).build();
@@ -134,7 +132,8 @@ public class FilesystemPackageManagerTests {
     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");
+    assertThat(exception.getMessage()).contains("Error reading package.");
+    assertThat(exception.getCause().getMessage()).contains("Timeout waiting for lock file deletion");
   }
 
   private static Thread lockWaitAndDelete(String path, String lockFileName, int seconds)  {
@@ -156,31 +155,34 @@ public class FilesystemPackageManagerTests {
 
 
   @Test
-  //@EnabledOnOs(OS.LINUX)
+  public void testReadFromCacheOnlyWaitsForLockDelete() throws IOException, InterruptedException {
-  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());
 
+    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";
+
     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();
 
-    Thread lockingThread = lockWaitAndDelete(pcmPath, lockFileName, 10);
+    final ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1);
+    executor.schedule(() -> {
+      lockFile.delete();
+    }, 5, TimeUnit.SECONDS);
 
-    Thread.sleep(200);
+    NpmPackage npmPackage = pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3");
-    IOException ioException = assertThrows(IOException.class, () -> { pcm.loadPackageFromCacheOnly("example.fhir.uv.myig", "1.2.3"); });
+
+    assertThat(npmPackage.id()).isEqualTo("example.fhir.uv.myig");
 
 
-    ioException.printStackTrace();
-
-    lockingThread.join();
   }
 
   /**
@@ -196,11 +198,123 @@ public class FilesystemPackageManagerTests {
     return params.stream();
   }
 
+  private void createDummyTemp(File cacheDirectory, String lowerCase) throws IOException {
+    createDummyPackage(cacheDirectory, lowerCase);
+  }
+
+  private void createDummyPackage(File cacheDirectory, String packageName, String packageVersion) throws IOException {
+    String directoryName = packageName + "#" + packageVersion;
+    createDummyPackage(cacheDirectory, directoryName);
+  }
+
+  private static void createDummyPackage(File cacheDirectory, String directoryName) throws IOException {
+    File packageDirectory = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), directoryName);
+    packageDirectory.mkdirs();
+
+    File dummyContentFile = ManagedFileAccess.file(packageDirectory.getAbsolutePath(), "dummy.txt");
+    FileWriter wr = new FileWriter(dummyContentFile);
+    wr.write("Ain't nobody here but us chickens");
+    wr.flush();
+    wr.close();
+  }
+
+  private void assertThatDummyTempExists(File cacheDirectory, String dummyTempPackage) throws IOException {
+    File dummyTempDirectory = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), dummyTempPackage);
+    assertThat(dummyTempDirectory).exists();
+
+    File dummyContentFile = ManagedFileAccess.file(dummyTempDirectory.getAbsolutePath(), "dummy.txt");
+    assertThat(dummyContentFile).exists();
+  }
+
+  @Test
+  public void testCreatesIniIfDoesntExistAndCacheStaysIntact() throws IOException {
+    File cacheDirectory = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest"));
+    File cacheIni = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "packages.ini");
+
+    createDummyPackage(cacheDirectory, "example.fhir.uv.myig", "1.2.3");
+
+    String dummyTempPackage = UUID.randomUUID().toString().toLowerCase();
+    createDummyTemp(cacheDirectory, dummyTempPackage);
+    assertThatDummyTempExists(cacheDirectory, dummyTempPackage);
+
+    assertThat(cacheIni).doesNotExist();
+    FilesystemPackageCacheManager filesystemPackageCacheManager = new FilesystemPackageCacheManager.Builder().withCacheFolder(cacheDirectory.getAbsolutePath()).build();
+    assertInitializedTestCacheIsValid(cacheDirectory, true);
+  }
+
+
+
+  @Test
+  public void testClearsCacheIfVersionIsWrong() throws IOException {
+    File cacheDirectory = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest"));
+    File cacheIni = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "packages.ini");
+
+    createDummyPackage(cacheDirectory, "example.fhir.uv.myig", "1.2.3");
+    String dummyTempPackage = UUID.randomUUID().toString().toLowerCase();
+    createDummyTemp(cacheDirectory, dummyTempPackage);
+    assertThatDummyTempExists(cacheDirectory, dummyTempPackage);
+
+
+    IniFile ini = new IniFile(cacheIni.getAbsolutePath());
+    ini.setStringProperty("cache", "version", "2", null);
+    ini.save();
+
+    assertThat(cacheIni).exists();
+    FilesystemPackageCacheManager filesystemPackageCacheManager = new FilesystemPackageCacheManager.Builder().withCacheFolder(cacheDirectory.getAbsolutePath()).build();
+    assertInitializedTestCacheIsValid(cacheDirectory, false);
+  }
+
+  @Test
+  public void testCacheStaysIntactIfVersionIsTheSame() throws IOException {
+    File cacheDirectory = ManagedFileAccess.fromPath(Files.createTempDirectory("fpcm-multithreadingTest"));
+    File cacheIni = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "packages.ini");
+
+    createDummyPackage(cacheDirectory, "example.fhir.uv.myig", "1.2.3");
+    String dummyTempPackage = UUID.randomUUID().toString().toLowerCase();
+    createDummyTemp(cacheDirectory, dummyTempPackage);
+    assertThatDummyTempExists(cacheDirectory, dummyTempPackage);
+
+
+    IniFile ini = new IniFile(cacheIni.getAbsolutePath());
+    ini.setStringProperty("cache", "version", "3", null);
+    ini.save();
+
+    assertThat(cacheIni).exists();
+    FilesystemPackageCacheManager filesystemPackageCacheManager = new FilesystemPackageCacheManager.Builder().withCacheFolder(cacheDirectory.getAbsolutePath()).build();
+    assertInitializedTestCacheIsValid(cacheDirectory, true);
+  }
+
+  private void assertInitializedTestCacheIsValid(File cacheDirectory, boolean dummyPackageShouldExist) throws IOException {
+    assertThat(cacheDirectory).exists();
+    File iniFile = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "packages.ini");
+    assertThat(ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "packages.ini")).exists();
+    IniFile ini = new IniFile(iniFile.getAbsolutePath());
+    String version = ini.getStringProperty("cache", "version");
+    assertThat(version).isEqualTo("3");
+
+    File[] files = cacheDirectory.listFiles();
+    if (dummyPackageShouldExist) {
+      // Check that only packages.ini and our dummy package are in the cache. Our previous temp should be deleted.
+      assertThat(files).hasSize(2); // packages.ini and example.fhir.uv.myig#1.2.3 (directory)
+
+      File dummyPackage = ManagedFileAccess.file(cacheDirectory.getAbsolutePath(), "example.fhir.uv.myig#1.2.3");
+      assertThat(dummyPackage).exists();
+
+      File dummyContentFile = ManagedFileAccess.file(dummyPackage.getAbsolutePath(), "dummy.txt");
+      assertThat(dummyContentFile).exists();
+    } else {
+      // Check that only packages.ini is in the cache.
+      assertThat(files).hasSize(1);
+    }
+
+
+  }
+
   @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();
 

org.hl7.fhir.validation.cli/pom.xml

@@ -5,7 +5,7 @@
     <parent>
         <groupId>ca.uhn.hapi.fhir</groupId>
         <artifactId>org.hl7.fhir.core</artifactId>
-        <version>6.3.24-SNAPSHOT</version>
+        <version>6.3.26-SNAPSHOT</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

org.hl7.fhir.validation/pom.xml

@@ -5,7 +5,7 @@
     <parent>
         <groupId>ca.uhn.hapi.fhir</groupId>
         <artifactId>org.hl7.fhir.core</artifactId>
-        <version>6.3.24-SNAPSHOT</version>
+        <version>6.3.26-SNAPSHOT</version>
         <relativePath>../pom.xml</relativePath>
     </parent>
 

pom.xml

@@ -14,7 +14,7 @@
     HAPI FHIR
     -->
     <artifactId>org.hl7.fhir.core</artifactId>
-    <version>6.3.24-SNAPSHOT</version>
+    <version>6.3.26-SNAPSHOT</version>
     <packaging>pom</packaging>
 
     <properties>