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
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,28 @@
*/
package org.jvnet.hudson.test;

import hudson.FilePath;

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;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
* 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 @@ -116,13 +111,34 @@ public synchronized void disposeAsync() {
new Thread("Disposing "+base) {
@Override
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 (Stream<Path> children = Files.list(p)) {
throw new IOException(children.map(Path::toString).collect(Collectors.joining(" ")), x);
}
}
}

}