From e89db465e4b83ab36c780ba7ca6285cc79a42a16 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 18 Sep 2024 15:11:50 -0400 Subject: [PATCH 1/2] Improved use of `java.io.tmpdir` --- .../jvnet/hudson/test/InboundAgentRule.java | 13 +++- .../hudson/test/JavaNetReverseProxy2.java | 12 +++- .../jvnet/hudson/test/RealJenkinsRule.java | 69 ++++++++++++------- .../test/TemporaryDirectoryAllocator.java | 8 ++- .../test/UnitTestSupportingPluginManager.java | 2 +- .../test/recipes/WithPluginManager.java | 2 +- 6 files changed, 76 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/jvnet/hudson/test/InboundAgentRule.java b/src/main/java/org/jvnet/hudson/test/InboundAgentRule.java index 06ef862f8..80d769fb9 100644 --- a/src/main/java/org/jvnet/hudson/test/InboundAgentRule.java +++ b/src/main/java/org/jvnet/hudson/test/InboundAgentRule.java @@ -43,6 +43,8 @@ import java.io.IOException; import java.io.Serializable; import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -440,6 +442,11 @@ public boolean isAlive(String name) { } } procs.clear(); + try { + FileUtils.deleteDirectory(Path.of(System.getProperty("java.io.tmpdir"), "agent-work-dirs").toFile()); + } catch (IOException x) { + LOGGER.log(Level.WARNING, null, x); + } } /** @@ -465,7 +472,7 @@ private static AgentArguments getAgentArguments(JenkinsRule r, String name) thro if (!launcher.getWorkDirSettings().isDisabled()) { commandLineArgs = launcher.getWorkDirSettings().toCommandLineArgs(c); } - File agentJar = new File(r.jenkins.getRootDir(), "agent.jar"); + File agentJar = new File(System.getProperty("java.io.tmpdir"), "agent.jar"); if (!agentJar.isFile()) { FileUtils.copyURLToFile(new Slave.JnlpJar("agent.jar").getURL(), agentJar); } @@ -509,7 +516,9 @@ private static Slave createAgentJR(JenkinsRule r, Options options) throws Descri } JNLPLauncher launcher = new JNLPLauncher(options.getTunnel()); launcher.setWebSocket(options.isWebSocket()); - DumbSlave s = new DumbSlave(options.getName(), new File(r.jenkins.getRootDir(), "agent-work-dirs/" + options.getName()).getAbsolutePath(), launcher); + var agentWorkDirs = Path.of(System.getProperty("java.io.tmpdir"), "agent-work-dirs"); + Files.createDirectories(agentWorkDirs); + DumbSlave s = new DumbSlave(options.getName(), Files.createTempDirectory(agentWorkDirs, options.getName()).toString(), launcher); s.setLabelString(options.getLabel()); s.setRetentionStrategy(RetentionStrategy.NOOP); r.jenkins.addNode(s); diff --git a/src/main/java/org/jvnet/hudson/test/JavaNetReverseProxy2.java b/src/main/java/org/jvnet/hudson/test/JavaNetReverseProxy2.java index 93f7478dc..03a5ad392 100644 --- a/src/main/java/org/jvnet/hudson/test/JavaNetReverseProxy2.java +++ b/src/main/java/org/jvnet/hudson/test/JavaNetReverseProxy2.java @@ -34,6 +34,17 @@ public class JavaNetReverseProxy2 extends HttpServlet { public JavaNetReverseProxy2(File cacheFolder) throws Exception { this.cacheFolder = cacheFolder; cacheFolder.mkdirs(); + Runtime.getRuntime().addShutdownHook(new Thread("delete " + cacheFolder) { + @Override + public void run() { + try { + Util.deleteRecursive(cacheFolder); + } catch (IOException x) { + x.printStackTrace(); + } + } + }); + QueuedThreadPool qtp = new QueuedThreadPool(); qtp.setName("Jetty (JavaNetReverseProxy)"); server = new Server(qtp); @@ -90,7 +101,6 @@ private String getMimeType(String path) { */ public static synchronized JavaNetReverseProxy2 getInstance() throws Exception { if (INSTANCE == null) { - // TODO: think of a better location --- ideally inside the target/ dir so that clean would wipe them out INSTANCE = new JavaNetReverseProxy2( new File(new File(System.getProperty("java.io.tmpdir")), "jenkins.io-cache2")); } diff --git a/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java b/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java index 2640b50a5..7a746291d 100644 --- a/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java +++ b/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java @@ -62,6 +62,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.nio.file.attribute.FileAttribute; import java.security.MessageDigest; import java.util.ArrayList; import java.util.Enumeration; @@ -99,6 +100,7 @@ import org.apache.commons.io.FileUtils; import org.junit.AssumptionViolatedException; import org.junit.rules.DisableOnDebug; +import org.junit.rules.TemporaryFolder; import org.junit.rules.TestRule; import org.junit.rules.Timeout; import org.junit.runner.Description; @@ -193,7 +195,7 @@ public final class RealJenkinsRule implements TestRule { Process proc; - private File portFile; + private Path portFile; private Map loggers = new HashMap<>(); @@ -504,27 +506,31 @@ public static List getJacocoAgentOptions() { return new Statement() { @Override public void evaluate() throws Throwable { System.out.println("=== Starting " + description); - if (war == null) { - war = findJenkinsWar(); - } - if (home.get() != null) { + try { + jenkinsOptions("--webroot=" + createTempDirectory("webroot"), "--pluginroot=" + createTempDirectory("pluginroot")); + if (war == null) { + war = findJenkinsWar(); + } + if (home.get() != null) { + try { + base.evaluate(); + } finally { + stopJenkins(); + } + return; + } try { + home.set(tmp.allocate()); + if (!prepareHomeLazily) { + provision(); + } base.evaluate(); } finally { stopJenkins(); } - return; - } - try { - home.set(tmp.allocate()); - if (!prepareHomeLazily) { - provision(); - } - base.evaluate(); } finally { - stopJenkins(); try { - deprovision(); + tmp.dispose(); } catch (Exception x) { LOGGER.log(Level.WARNING, null, x); } @@ -652,6 +658,17 @@ public void deprovision() throws Exception { provisioned = false; } + /** + * Creates a temporary directory. + * Unlike {@link Files#createTempDirectory(String, FileAttribute...)} + * this will be cleaned up after the test exits (like {@link TemporaryFolder}), + * and will honor {@code java.io.tmpdir} set by Surefire + * after {@code StaticProperty.JAVA_IO_TMPDIR} has been initialized. + */ + public Path createTempDirectory(String prefix) throws IOException { + return tmp.allocate(prefix).toPath(); + } + /** * Returns true if the Jenkins process is alive. */ @@ -752,15 +769,18 @@ public void startJenkins() throws Throwable { if (prepareHomeLazily && !provisioned) { provision(); } + var metadata = createTempDirectory("RealJenkinsRule"); + var cpFile = metadata.resolve("cp.txt"); String cp = System.getProperty("java.class.path"); Files.writeString( - getHome().toPath().resolve("RealJenkinsRule-cp.txt"), + cpFile, Stream.of(cp.split(File.pathSeparator)).collect(Collectors.joining(System.lineSeparator())), StandardCharsets.UTF_8); List argv = new ArrayList<>(List.of( new File(javaHome != null ? javaHome : System.getProperty("java.home"), "bin/java").getAbsolutePath(), "-ea", "-Dhudson.Main.development=true", + "-DRealJenkinsRule.classpath=" + cpFile, "-DRealJenkinsRule.location=" + RealJenkinsRule.class.getProtectionDomain().getCodeSource().getLocation(), "-DRealJenkinsRule.description=" + description, "-DRealJenkinsRule.token=" + token)); @@ -768,9 +788,12 @@ public void startJenkins() throws Throwable { for (Map.Entry e : loggers.entrySet()) { argv.add("-D" + REAL_JENKINS_RULE_LOGGING + e.getKey() + "=" + e.getValue().getName()); } - portFile = File.createTempFile("jenkins-port", ".txt", getHome()); - Files.delete(portFile.toPath()); + portFile = metadata.resolve("jenkins-port.txt"); argv.add("-Dwinstone.portFileName=" + portFile); + var tmp = System.getProperty("java.io.tmpdir"); + if (tmp != null) { + argv.add("-Djava.io.tmpdir=" + tmp); + } boolean debugging = new DisableOnDebug(null).isDebugging(); if (debugging) { argv.add("-agentlib:jdwp=transport=dt_socket" @@ -821,7 +844,7 @@ public void startJenkins() throws Throwable { proc = null; throw new IOException("Jenkins process terminated prematurely with exit code " + exitValue); } - if (port == 0 && portFile != null && portFile.exists()) { + if (port == 0 && portFile != null && Files.isRegularFile(portFile)) { port = readPort(portFile); } if (port != 0) { @@ -904,13 +927,13 @@ private void addTimeout() { } } - private static int readPort(File portFile) throws IOException { - String s = Files.readString(portFile.toPath(), StandardCharsets.UTF_8); + private static int readPort(Path portFile) throws IOException { + String s = Files.readString(portFile, StandardCharsets.UTF_8); // Work around to non-atomic write of port value in Winstone releases prior to 6.1. // TODO When Winstone 6.2 has been widely adopted, this can be deleted. if (s.isEmpty()) { - LOGGER.warning(() -> String.format("PortFile: %s exists, but value is still not written", portFile.getAbsolutePath())); + LOGGER.warning(() -> String.format("PortFile: %s exists, but value is still not written", portFile)); return 0; } @@ -1170,7 +1193,7 @@ public static final class Init2 { public static void run(Object jenkins) throws Exception { Object pluginManager = jenkins.getClass().getField("pluginManager").get(jenkins); ClassLoader uberClassLoader = (ClassLoader) pluginManager.getClass().getField("uberClassLoader").get(pluginManager); - ClassLoader tests = new URLClassLoader(Files.readAllLines(Paths.get(System.getenv("JENKINS_HOME"), "RealJenkinsRule-cp.txt"), StandardCharsets.UTF_8).stream().map(Init2::pathToURL).toArray(URL[]::new), uberClassLoader); + ClassLoader tests = new URLClassLoader(Files.readAllLines(Paths.get(System.getProperty("RealJenkinsRule.classpath")), StandardCharsets.UTF_8).stream().map(Init2::pathToURL).toArray(URL[]::new), uberClassLoader); tests.loadClass("org.jvnet.hudson.test.RealJenkinsRule$Endpoint").getMethod("register").invoke(null); } diff --git a/src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java b/src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java index 818a992aa..cd7d4aea9 100644 --- a/src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java +++ b/src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java @@ -81,9 +81,13 @@ public TemporaryDirectoryAllocator() { * This directory will be wiped out when {@link TemporaryDirectoryAllocator} gets disposed. * When this method returns, the directory already exists. */ - public synchronized File allocate() throws IOException { + public File allocate() throws IOException { + return allocate(withoutSpace ? "jkh" : "j h"); + } + + synchronized File allocate(String name) throws IOException { try { - File f = Files.createTempDirectory(base.toPath(), (withoutSpace ? "jkh" : "j h")).toFile(); + File f = Files.createTempDirectory(base.toPath(), name).toFile(); tmpDirectories.add(f); return f; } catch (IOException e) { diff --git a/src/main/java/org/jvnet/hudson/test/UnitTestSupportingPluginManager.java b/src/main/java/org/jvnet/hudson/test/UnitTestSupportingPluginManager.java index 55a66b96c..4bafed96a 100644 --- a/src/main/java/org/jvnet/hudson/test/UnitTestSupportingPluginManager.java +++ b/src/main/java/org/jvnet/hudson/test/UnitTestSupportingPluginManager.java @@ -63,7 +63,7 @@ public class UnitTestSupportingPluginManager extends PluginManager { public UnitTestSupportingPluginManager(File rootDir) { - super((ServletContext) null, new File(rootDir, "plugins")); + super((ServletContext) null, rootDir); } /** @see LocalPluginManager#loadBundledPlugins */ diff --git a/src/main/java/org/jvnet/hudson/test/recipes/WithPluginManager.java b/src/main/java/org/jvnet/hudson/test/recipes/WithPluginManager.java index d1816d369..ec771873f 100644 --- a/src/main/java/org/jvnet/hudson/test/recipes/WithPluginManager.java +++ b/src/main/java/org/jvnet/hudson/test/recipes/WithPluginManager.java @@ -90,7 +90,7 @@ public void setup(JenkinsRule jenkinsRule, WithPluginManager recipe) throws Exce public void decorateHome(JenkinsRule jenkinsRule, File home) throws Exception { Class c = recipe.value(); Constructor ctr = c.getDeclaredConstructor(File.class); - jenkinsRule.setPluginManager(ctr.newInstance(home)); + jenkinsRule.setPluginManager(ctr.newInstance(new File(home, "plugins"))); } } } From f3f72c7999c0112ad82a13e6e31395a70380bd32 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 19 Sep 2024 08:38:34 -0400 Subject: [PATCH 2/2] `InboundAgentRule.after` should delete just those work dirs it created --- .../jvnet/hudson/test/InboundAgentRule.java | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/jvnet/hudson/test/InboundAgentRule.java b/src/main/java/org/jvnet/hudson/test/InboundAgentRule.java index 80d769fb9..952dbdd9d 100644 --- a/src/main/java/org/jvnet/hudson/test/InboundAgentRule.java +++ b/src/main/java/org/jvnet/hudson/test/InboundAgentRule.java @@ -48,10 +48,12 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.UUID; import java.util.jar.JarFile; import java.util.logging.Level; @@ -85,6 +87,7 @@ public final class InboundAgentRule extends ExternalResource { private final String id = UUID.randomUUID().toString(); private final Map procs = Collections.synchronizedMap(new HashMap<>()); + private final Set workDirs = Collections.synchronizedSet(new HashSet<>()); /** * The options used to (re)start an inbound agent. @@ -280,6 +283,7 @@ public Slave createAgent(@NonNull JenkinsRule r, @CheckForNull String name) thro */ public Slave createAgent(@NonNull JenkinsRule r, Options options) throws Exception { Slave s = createAgentJR(r, options); + workDirs.add(s.getRemoteFS()); if (options.isStart()) { start(r, options); } @@ -291,8 +295,9 @@ public void createAgent(@NonNull RealJenkinsRule rr, @CheckForNull String name) } public void createAgent(@NonNull RealJenkinsRule rr, Options options) throws Throwable { - String name = rr.runRemotely(InboundAgentRule::createAgentRJR, options); - options.name = name; + var nameAndWorkDir = rr.runRemotely(InboundAgentRule::createAgentRJR, options); + options.name = nameAndWorkDir[0]; + workDirs.add(nameAndWorkDir[1]); if (options.isStart()) { start(rr, options); } @@ -425,6 +430,7 @@ public boolean isAlive(String name) { return proc != null && proc.isAlive(); } + @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "test code") @Override protected void after() { for (var entry : procs.entrySet()) { String name = entry.getKey(); @@ -442,10 +448,13 @@ public boolean isAlive(String name) { } } procs.clear(); - try { - FileUtils.deleteDirectory(Path.of(System.getProperty("java.io.tmpdir"), "agent-work-dirs").toFile()); - } catch (IOException x) { - LOGGER.log(Level.WARNING, null, x); + for (var workDir : workDirs) { + LOGGER.info(() -> "Deleting " + workDir); + try { + FileUtils.deleteDirectory(new File(workDir)); + } catch (IOException x) { + LOGGER.log(Level.WARNING, null, x); + } } } @@ -504,9 +513,9 @@ private static void waitForAgentOffline(JenkinsRule r, String name) throws Inter } } - private static String createAgentRJR(JenkinsRule r, Options options) throws Throwable { - createAgentJR(r, options); - return options.getName(); + private static String[] createAgentRJR(JenkinsRule r, Options options) throws Throwable { + var agent = createAgentJR(r, options); + return new String[] {options.getName(), agent.getRemoteFS()}; } @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "just for test code") @@ -516,9 +525,7 @@ private static Slave createAgentJR(JenkinsRule r, Options options) throws Descri } JNLPLauncher launcher = new JNLPLauncher(options.getTunnel()); launcher.setWebSocket(options.isWebSocket()); - var agentWorkDirs = Path.of(System.getProperty("java.io.tmpdir"), "agent-work-dirs"); - Files.createDirectories(agentWorkDirs); - DumbSlave s = new DumbSlave(options.getName(), Files.createTempDirectory(agentWorkDirs, options.getName()).toString(), launcher); + DumbSlave s = new DumbSlave(options.getName(), Files.createTempDirectory(Path.of(System.getProperty("java.io.tmpdir")), options.getName() + "-work").toString(), launcher); s.setLabelString(options.getLabel()); s.setRetentionStrategy(RetentionStrategy.NOOP); r.jenkins.addNode(s);