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

Fixing file leaks in tests (preadapting to proposed TemporaryDirectoryAllocator.dispose change) #8006

Merged
Merged
32 changes: 20 additions & 12 deletions test/src/test/java/hudson/LauncherTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -190,23 +190,25 @@ private static final class QuietBatchFile extends BatchFile {
@Issue("JENKINS-52729")
@Test public void remotable() throws Exception {
File log = new File(rule.jenkins.root, "log");
TaskListener listener = new RemotableBuildListener(log);
Launcher.ProcStarter ps = rule.createOnlineSlave().createLauncher(listener).launch();
if (Functions.isWindows()) {
ps.cmds("cmd", "/c", "echo", "hello");
} else {
ps.cmds("echo", "hello");
try (var listener = new RemotableBuildListener(log)) {
Launcher.ProcStarter ps = rule.createOnlineSlave().createLauncher(listener).launch();
Copy link
Member Author

Choose a reason for hiding this comment

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

(hide whitespace to review)

if (Functions.isWindows()) {
ps.cmds("cmd", "/c", "echo", "hello");
} else {
ps.cmds("echo", "hello");
}
assertEquals(0, ps.stdout(listener).join());
assertThat(Files.readString(log.toPath(), StandardCharsets.UTF_8).replace("\r\n", "\n"),
containsString("[master → slave0] $ " + (Functions.isWindows() ? "cmd /c " : "") + "echo hello\n" +
"[master → slave0] hello"));
}
assertEquals(0, ps.stdout(listener).join());
assertThat(Files.readString(log.toPath(), StandardCharsets.UTF_8).replace("\r\n", "\n"),
containsString("[master → slave0] $ " + (Functions.isWindows() ? "cmd /c " : "") + "echo hello\n" +
"[master → slave0] hello"));
}

private static class RemotableBuildListener implements BuildListener {
private static class RemotableBuildListener implements BuildListener, AutoCloseable {
private static final long serialVersionUID = 1;
/** location of log file streamed to by multiple sources */
private final File logFile;
private OutputStream fos;
/** records allocation & deserialization history; e.g., {@code master → agentName} */
private final String id;
private transient PrintStream logger;
Expand All @@ -223,7 +225,6 @@ private RemotableBuildListener(File logFile, String id) {
@NonNull
@Override public PrintStream getLogger() {
if (logger == null) {
final OutputStream fos;
try {
fos = new FileOutputStream(logFile, true);
logger = new PrintStream(new LineTransformationOutputStream() {
Expand All @@ -244,6 +245,13 @@ private Object writeReplace() {
String name = Channel.current().getName();
return new RemotableBuildListener(logFile, id + " → " + name);
}

@Override
public void close() throws Exception {
if (fos != null) {
fos.close();
}
}
}

@Issue("JENKINS-52729")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import hudson.tasks.ArtifactArchiver;
import hudson.tasks.BatchFile;
import hudson.tasks.Shell;
import hudson.util.StreamTaskListener;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileNotFoundException;
Expand Down Expand Up @@ -919,6 +920,8 @@ public void junctionAndSymlink_outsideWorkspace_areNotAllowed_windowsJunction()
List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, contains("intermediateFolder/public2.key"));
}
// Explicitly delete everything including junctions, which TemporaryDirectoryAllocator.dispose may have trouble with:
new Launcher.LocalLauncher(StreamTaskListener.fromStderr()).launch().cmds("cmd", "/c", "rmdir", "/s", "/q", j.jenkins.getRootDir().getAbsolutePath()).start().join();
Copy link
Member

Choose a reason for hiding this comment

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

why this cleanup? isn't this exactly what the test harness does (and if that can not clean up because the file is locked why would rmdir be able to?

Copy link
Member Author

Choose a reason for hiding this comment

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

}

private List<String> getListOfEntriesInDownloadedZip(UnexpectedPage zipPage) throws Exception {
Expand Down
25 changes: 15 additions & 10 deletions test/src/test/java/hudson/model/QueueTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -916,18 +916,23 @@ public void save(){
if (e.isIdle()) {
assertTrue("Node went to idle before project had" + project2.getDisplayName() + " been started", v.isDone());
}
Thread.sleep(1000);
Thread.sleep(1000);
}
if (project2.getLastBuild() != null)
return;
Queue.getInstance().cancel(projectError); // cancel job which cause dead of executor
while (!e.isIdle()) { //executor should take project2 from queue
Thread.sleep(1000);
if (project2.getLastBuild() == null) {
Queue.getInstance().cancel(projectError); // cancel job which cause dead of executor
while (!e.isIdle()) { //executor should take project2 from queue
Thread.sleep(1000);
}
//project2 should not be in pendings
List<Queue.BuildableItem> items = Queue.getInstance().getPendingItems();
for (Queue.BuildableItem item : items) {
assertNotEquals("Project " + project2.getDisplayName() + " stuck in pendings", item.task.getName(), project2.getName());
}
}
//project2 should not be in pendings
List<Queue.BuildableItem> items = Queue.getInstance().getPendingItems();
for (Queue.BuildableItem item : items) {
assertNotEquals("Project " + project2.getDisplayName() + " stuck in pendings", item.task.getName(), project2.getName());
for (var p : r.jenkins.allItems(FreeStyleProject.class)) {
for (var b : p.getBuilds()) {
r.waitForCompletion(b);
}
}
}

Expand Down