From 2b994673cf095be0a6c899502e3f5f31a398960d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Bulej?= Date: Wed, 4 Dec 2024 19:16:11 +0100 Subject: [PATCH 1/4] Use ConsoleHandler instead of StreamHandler in logging setup This avoids stderr being closed by the StreamHandler on shutdown, preventing printing messages to stderr from shutdown hooks. --- .../src/main/java/org/renaissance/core/Logging.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/renaissance-core/src/main/java/org/renaissance/core/Logging.java b/renaissance-core/src/main/java/org/renaissance/core/Logging.java index 18346e2a..6310e117 100644 --- a/renaissance-core/src/main/java/org/renaissance/core/Logging.java +++ b/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() {} From 5a31eb9445315f54a53e49cf59ceebdd400b3316 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Bulej?= Date: Wed, 4 Dec 2024 19:10:10 +0100 Subject: [PATCH 2/4] Centralize cleanup tasks and close created URLClassLoader instances This ensures that library JAR files stored in the launcher and harness scratch directories are closed before removing the scratch directory roots. This was a problem on NFS, where the open files could not be unlinked from the directory (NFS kept around '.nfs*' temporaries), preventing the removal of the directories. --- .../java/org/renaissance/core/Cleaner.java | 76 +++++++++++++++++++ .../java/org/renaissance/core/DirUtils.java | 22 ++---- .../org/renaissance/core/ModuleLoader.java | 2 +- 3 files changed, 84 insertions(+), 16 deletions(-) create mode 100644 renaissance-core/src/main/java/org/renaissance/core/Cleaner.java diff --git a/renaissance-core/src/main/java/org/renaissance/core/Cleaner.java b/renaissance-core/src/main/java/org/renaissance/core/Cleaner.java new file mode 100644 index 00000000..fcd10f93 --- /dev/null +++ b/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 registeredCloseables = new LinkedHashSet<>(); + private static Set registeredPaths = new LinkedHashSet<>(); + + private Cleaner() {} + + static Path deleteOnExit(Path path) { + return register(registeredPaths, path); + } + + static T closeOnExit(T closeable) { + return register(registeredCloseables, closeable); + } + + private synchronized static T register(Set 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 closeables; + Set 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 { + void accept(T object) throws IOException; + } + + private static > C cleanupEach(C items, String name, Action 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)); + } + +} diff --git a/renaissance-core/src/main/java/org/renaissance/core/DirUtils.java b/renaissance-core/src/main/java/org/renaissance/core/DirUtils.java index fcb53c35..ce7981e8 100644 --- a/renaissance-core/src/main/java/org/renaissance/core/DirUtils.java +++ b/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 do 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(); - } - } diff --git a/renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java b/renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java index 49f294eb..8f271083 100644 --- a/renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java +++ b/renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java @@ -158,7 +158,7 @@ ClassLoader createClassLoaderForModule(String name) throws ModuleLoadingExceptio name, filePaths.size(), makeClassPath(filePaths) )); - return new URLClassLoader(urls, thisClass.getClassLoader()); + return Cleaner.closeOnExit(new URLClassLoader(urls, thisClass.getClassLoader())); } catch (IOException e) { // Just wrap the underlying IOException. From fc3a8008f93492ec6dfaa03a8b7166daff65540f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20T=C5=AFma?= Date: Thu, 5 Dec 2024 07:35:43 +0100 Subject: [PATCH 3/4] I wanted to have a commit too :-) --- .../src/main/java/org/renaissance/core/DirUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/renaissance-core/src/main/java/org/renaissance/core/DirUtils.java b/renaissance-core/src/main/java/org/renaissance/core/DirUtils.java index ce7981e8..9f845a76 100644 --- a/renaissance-core/src/main/java/org/renaissance/core/DirUtils.java +++ b/renaissance-core/src/main/java/org/renaissance/core/DirUtils.java @@ -39,7 +39,7 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx 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 do delete this directory anyway. + // that we will not be able to delete this directory anyway. throw exc; } From c83e0ac31beefa19739a4c33d88393dd5b68bca3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Bulej?= Date: Thu, 5 Dec 2024 11:38:30 +0100 Subject: [PATCH 4/4] Comment on explicit closing of URLClassLoader instances Specifically, why we need it for benchmarks but not for plugins. --- .../main/java/org/renaissance/core/BenchmarkSuite.java | 4 ++++ .../src/main/java/org/renaissance/core/ModuleLoader.java | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/renaissance-core/src/main/java/org/renaissance/core/BenchmarkSuite.java b/renaissance-core/src/main/java/org/renaissance/core/BenchmarkSuite.java index 462a4d64..69588ffc 100644 --- a/renaissance-core/src/main/java/org/renaissance/core/BenchmarkSuite.java +++ b/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); } diff --git a/renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java b/renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java index 8f271083..b352a7af 100644 --- a/renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java +++ b/renaissance-core/src/main/java/org/renaissance/core/ModuleLoader.java @@ -158,6 +158,14 @@ ClassLoader createClassLoaderForModule(String name) throws ModuleLoadingExceptio name, filePaths.size(), makeClassPath(filePaths) )); + // + // 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) {