Skip to content

Commit

Permalink
Be more defensive about freeing resources
Browse files Browse the repository at this point in the history
Although this *should* not happen, just free up the watches when the
ContainerStep finishes for good measure.
  • Loading branch information
Martin Sander committed Aug 8, 2017
1 parent 76ed602 commit c9143f8
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

import static org.csanchez.jenkins.plugins.kubernetes.pipeline.Constants.*;

import java.io.Closeable;
import java.io.IOException;
import java.io.InterruptedIOException;
import java.io.OutputStream;
import java.io.PrintStream;
import java.io.Serializable;
Expand Down Expand Up @@ -60,7 +60,7 @@
* the Jenkins slave to execute commands.
*
*/
public class ContainerExecDecorator extends LauncherDecorator implements Serializable {
public class ContainerExecDecorator extends LauncherDecorator implements Serializable, Closeable {

private static final long serialVersionUID = 4419929753433397655L;
private static final long DEFAULT_CONTAINER_READY_TIMEOUT = 5;
Expand All @@ -70,6 +70,7 @@ public class ContainerExecDecorator extends LauncherDecorator implements Seriali
private static final Logger LOGGER = Logger.getLogger(ContainerExecDecorator.class.getName());

private final transient KubernetesClient client;
private final transient List<Closeable> closables = new ArrayList<>();
private final String podName;
private final String namespace;
private final String containerName;
Expand Down Expand Up @@ -195,7 +196,9 @@ public void onClose(int i, String s) {
}
doExec(watch, printStream, commands);

return new ContainerExecProc(watch, alive, finished, exitCodeOutputStream::getExitCode);
ContainerExecProc proc = new ContainerExecProc(watch, alive, finished, exitCodeOutputStream::getExitCode);
closables.add(proc);
return proc;
} catch (Exception e) {
closeWatch(watch);
throw e;
Expand Down Expand Up @@ -243,6 +246,17 @@ private void waitUntilContainerIsReady() throws IOException {
};
}

@Override
public void close() throws IOException {
for (Closeable closable : closables) {
try {
closable.close();
} catch (Exception e) {
LOGGER.log(Level.FINE, "failed to close {0}");
}
}
}

private static void doExec(ExecWatch watch, PrintStream out, String... statements) {
try {
out.print("Executing command: ");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static org.csanchez.jenkins.plugins.kubernetes.pipeline.Constants.*;

import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand All @@ -19,7 +20,7 @@
* Handle the liveness of the processes executed in containers, wait for them to finish and process exit codes.
*
*/
public class ContainerExecProc extends Proc {
public class ContainerExecProc extends Proc implements Closeable {

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

Expand Down Expand Up @@ -59,6 +60,8 @@ public void kill() throws IOException, InterruptedException {
watch.getInput().flush();
} catch (IOException e) {
LOGGER.log(Level.FINE, "Proc kill failed, ignoring", e);
} finally {
close();
}
}

Expand All @@ -73,12 +76,7 @@ public int join() throws IOException, InterruptedException {
LOGGER.log(Level.WARNING, "Error getting exit code", e);
return -1;
} finally {
//We are calling explicitly close, in order to cleanup websockets and threads (are not closed implicitly).
try {
watch.close();
} catch (Exception e) {
LOGGER.log(Level.INFO, "failed to close watch", e);
}
close();
}
}

Expand All @@ -97,4 +95,13 @@ public OutputStream getStdin() {
return watch.getInput();
}

@Override
public void close() throws IOException {
try {
//We are calling explicitly close, in order to cleanup websockets and threads (are not closed implicitly).
watch.close();
} catch (Exception e) {
LOGGER.log(Level.INFO, "failed to close watch", e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,22 +61,22 @@ public boolean start() throws Exception {
getContext().newBodyInvoker()
.withContext(BodyInvoker
.mergeLauncherDecorators(getContext().get(LauncherDecorator.class), decorator))
.withCallback(new ContainerExecCallback())
.withCallback(new ContainerExecCallback(decorator))
.start();
return false;
}

@Override
public void stop(Throwable cause) throws Exception {
LOGGER.log(Level.FINE, "Stopping container step.");
closeQuietly(client);
closeQuietly(client, decorator);
}

private void closeQuietly(Closeable... closeables) {
closeQuietly(getContext(), closeables);
}

private static class ContainerExecCallback extends BodyExecutionCallback {
private static class ContainerExecCallback extends BodyExecutionCallback.TailCall {

private static final long serialVersionUID = 6385838254761750483L;

Expand All @@ -85,17 +85,8 @@ private static class ContainerExecCallback extends BodyExecutionCallback {
private ContainerExecCallback(Closeable... closeables) {
this.closeables = closeables;
}

@Override
public void onSuccess(StepContext context, Object result) {
context.onSuccess(result);
closeQuietly(context, closeables);

}

@Override
public void onFailure(StepContext context, Throwable t) {
context.onFailure(t);
public void finished(StepContext context) {
closeQuietly(context, closeables);
}
}
Expand Down

0 comments on commit c9143f8

Please sign in to comment.