Merge pull request #465 from renaissance-benchmarks/topic/issue464

Close URLClassLoader instances to enable proper cleanup on NFS

renaissance-core/src/main/java/org/renaissance/core/BenchmarkSuite.java

@@ -283,6 +283,10 @@ private static ClassLoader createClassLoaderFromPaths(
       throw new ExtensionException("malformed URL(s) in classpath specification");
     }
 
+    //
+    // No need to explicitly close this URLClassLoader on exit, because it does not
+    // operate on files created by the harness that need to be deleted on exit.
+    //
     ClassLoader parent = ModuleLoader.class.getClassLoader();
     return new URLClassLoader(classPathUrls, parent);
   }

renaissance-core/src/main/java/org/renaissance/core/Cleaner.java

@@ -0,0 +1,76 @@
+package org.renaissance.core;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.LinkedHashSet;
+import java.util.Objects;
+import java.util.Set;
+
+final class Cleaner {
+  private static Set<Closeable> registeredCloseables = new LinkedHashSet<>();
+  private static Set<Path> registeredPaths = new LinkedHashSet<>();
+
+  private Cleaner() {}
+
+  static Path deleteOnExit(Path path) {
+    return register(registeredPaths, path);
+  }
+
+  static <T extends Closeable> T closeOnExit(T closeable) {
+    return register(registeredCloseables, closeable);
+  }
+
+  private synchronized static <T> T register(Set<? super T> registry, T item) {
+    if (registry == null) {
+      throw new IllegalStateException("Shutdown in progress");
+    } else {
+      registry.add(Objects.requireNonNull(item));
+    }
+
+    return item;
+  }
+
+  private static void run() {
+    Set<Closeable> closeables;
+    Set<Path> paths;
+
+    synchronized (Cleaner.class) {
+      closeables = registeredCloseables;
+      registeredCloseables = null;
+
+      paths = registeredPaths;
+      registeredPaths = null;
+    }
+
+    // Close closeables BEFORE deleting files/directories.
+    // In particular URLClassLoader instances which keep files open.
+    // Delete files/directories AFTER closing closeables.
+
+    cleanupEach(closeables, "close", Closeable::close).clear();
+    cleanupEach(paths, "delete", DirUtils::deleteRecursively).clear();
+  }
+
+  @FunctionalInterface
+  private interface Action<T> {
+    void accept(T object) throws IOException;
+  }
+
+  private static <T, C extends Iterable<T>> C cleanupEach(C items, String name, Action<T> action) {
+    for (T item : items) {
+      try {
+        action.accept(item);
+      } catch (IOException e) {
+        // Just a notification. This should be rare and is not critical.
+        System.err.format("warning: failed to %s %s on shutdown: %s\n", name, item, e);
+      }
+    }
+
+    return items;
+  }
+
+  static {
+    Runtime.getRuntime().addShutdownHook(new Thread(Cleaner::run));
+  }
+
+}

renaissance-core/src/main/java/org/renaissance/core/DirUtils.java

@@ -37,6 +37,12 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
       @Override
       public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
         if (deleteRoot || !rootDir.equals(dir)) {
+          if (exc != null) {
+            // There was an earlier failure with one of the children, which means
+            // that we will not be able to delete this directory anyway.
+            throw exc;
+          }
+
           Files.delete(dir);
         }
 
@@ -52,24 +58,10 @@ public static Path createScratchDirectory(Path base, String prefix, boolean keep
 
     Path scratchDir = createTempDirectory(base, tsPrefix).normalize();
     if (!keepOnExit) {
-      Runtime.getRuntime().addShutdownHook(new Thread (() -> {
-        logger.fine(() -> "Deleting scratch directory: " + printable(scratchDir));
-        try {
-          deleteRecursively(scratchDir);
-        } catch (IOException e) {
-          // Just a notification. This should be rare and is not critical.
-          logger.warning(String.format(
-            "Error deleting scratch directory %s: %s", printable(scratchDir), e.getMessage()
-          ));
-        }
-      }));
+      Cleaner.deleteOnExit(scratchDir);
     }
 
     return scratchDir;
   }
 
-  private static Path printable(Path path) {
-    return path.toAbsolutePath().normalize();
-  }
-
 }

renaissance-core/src/main/java/org/renaissance/core/Logging.java

@@ -1,8 +1,8 @@
 package org.renaissance.core;
 
+import java.util.logging.ConsoleHandler;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.logging.SimpleFormatter;
 import java.util.logging.StreamHandler;
 
 public final class Logging {
@@ -30,8 +30,8 @@ private static StreamHandler createHandler() {
       System.setProperty(FORMAT_PROPERTY, DEFAULT_FORMAT);
     }
 
-    // Create SimpleFormatter AFTER setting the system property.
-    return new StreamHandler(System.err, new SimpleFormatter());
+    // Creates SimpleFormatter AFTER setting the system property.
+    return new ConsoleHandler();
   }
 
   private Logging() {}

renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java

@@ -158,7 +158,15 @@ ClassLoader createClassLoaderForModule(String name) throws ModuleLoadingExceptio
           name, filePaths.size(), makeClassPath(filePaths)
         ));
 
-        return new URLClassLoader(urls, thisClass.getClassLoader());
+        //
+        // Make sure to explicitly close this URLClassLoader on exit, because it operates
+        // on files created by the harness in the scratch directory hierarchy that need to
+        // be deleted on exit. Leaving the class loader open keeps the library JAR files
+        // open, preventing removal of the scratch directories. This is because on NFS,
+        // deleting an open file produces a NFS temporary file in the same directory, and
+        // on Windows, an open file cannot be deleted at all.
+        //
+        return Cleaner.closeOnExit(new URLClassLoader(urls, thisClass.getClassLoader()));
 
       } catch (IOException e) {
         // Just wrap the underlying IOException.