Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplified TemporaryDirectoryAllocator.dispose #198

Merged
merged 11 commits into from
May 19, 2023
7 changes: 0 additions & 7 deletions src/main/java/org/jvnet/hudson/test/JenkinsRule.java
Original file line number Diff line number Diff line change
Expand Up @@ -510,13 +510,6 @@ public void after() throws Exception {
try {
env.dispose();
} finally {
// Hudson creates ClassLoaders for plugins that hold on to file descriptors of its jar files,
// but because there's no explicit dispose method on ClassLoader, they won't get GC-ed until
// at some later point, leading to possible file descriptor overflow. So encourage GC now.
// see http://bugs.sun.com/view_bug.do?bug_id=4950148
// TODO use URLClassLoader.close() in Java 7
jglick marked this conversation as resolved.
Show resolved Hide resolved
System.gc();

// restore defaultUseCache
if(Functions.isWindows()) {
URLConnection aConnection = new File(".").toURI().toURL().openConnection();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,27 @@
*/
package org.jvnet.hudson.test;

import hudson.FilePath;

import com.google.common.collect.Iterables;
jglick marked this conversation as resolved.
Show resolved Hide resolved
import java.io.File;
import java.io.IOException;
import java.nio.file.DirectoryNotEmptyException;
import java.nio.file.DirectoryStream;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.Path;
import java.util.HashSet;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
* Allocates temporary directories and cleans it up at the end.
* @author Kohsuke Kawaguchi
*/
public class TemporaryDirectoryAllocator {

private static final Logger LOGGER = Logger.getLogger(TemporaryDirectoryAllocator.class.getName());

/**
* Remember allocated directories to delete them later.
*/
Expand Down Expand Up @@ -85,25 +94,11 @@ public synchronized File allocate() throws IOException {
* Deletes all allocated temporary directories.
*/
public synchronized void dispose() throws IOException, InterruptedException {
// TODO when we bump the Jenkins dependency to 2.157
IOException x = null;
for (File dir : tmpDirectories) {
try {
new FilePath(dir).deleteRecursive();
} catch (IOException e) {
if (x == null) {
x = e;
}
else {
x.addSuppressed(e);
}
}
LOGGER.info(() -> "deleting " + dir);
delete(dir.toPath());
}
tmpDirectories.clear();
if (x != null) {
// do not wrap this pending JENKINS-60308
throw x;
}
}

/**
Expand All @@ -115,13 +110,34 @@ public synchronized void disposeAsync() {

new Thread("Disposing "+base) {
public void run() {
for (File dir : tbr)
for (File dir : tbr) {
LOGGER.info(() -> "deleting " + dir);
try {
new FilePath(dir).deleteRecursive();
} catch (IOException | InterruptedException e) {
e.printStackTrace();
delete(dir.toPath());
} catch (IOException e) {
LOGGER.log(Level.WARNING, null, e);
}
}
}
}.start();
}

private void delete(Path p) throws IOException {
LOGGER.fine(() -> "deleting " + p);
if (Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably also a junction which I think may be used in some test case and FIlePath respects.. (FIles.readAttributes and isOther - as a general well it is not a normal directory)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know much about junctions. Would they ever really be used in $JENKINS_HOME?

Copy link
Member

@jtnord jtnord Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure something uses them as we have specific code in Jenkins...
https://github.com/jenkinsci/jenkins/blob/715cdb15b3e855f8b6115aa02200860ed6caf49d/core/src/main/java/hudson/Util.java#L323-L343
I have not come across a real life example, but I'm guessing that it is for a reason?

Maybe it is just an easy way in Jenkins to put some subset of Jobs in a different drive / pool etc and would not actually be done inside a workspace, or a build directory by jenkins (I do not know if zipping on windows copies the Junction or not, so if a workspace was coppied from an agent where the build created a junction if that would be replcicated on a master).

ec2 plugin seems to use them for something?
https://github.com/jenkinsci/ec2-plugin/blob/20f0c2406d62ddbd0d005a2fb15f3f9bb95b3f20/src/main/java/hudson/os/WindowsUtil.java#L109-L121

and the workspacebrowser has a test to prevent traversing them IIRC (so there is at least one use in Jenkins where a workspace will have junctions) https://github.com/jenkinsci/jenkins/blob/9330635821cbcb430b4223b471692f2c9ae862db/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java#L617

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be some obscure use case in production servers, but I doubt this would affect functional tests. If there is some plugin which does create a junction during a test for some reason, then it can explicitly remove it before generic cleanup, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try (DirectoryStream<Path> children = Files.newDirectoryStream(p)) {
for (Path child : children) {
delete(child);
}
}
}
try {
Files.deleteIfExists(p);
} catch (DirectoryNotEmptyException x) {
try (DirectoryStream<Path> children = Files.newDirectoryStream(p)) {
throw new IOException(Iterables.toString(children), x);
}
jglick marked this conversation as resolved.
Show resolved Hide resolved
}
}

}