Skip to content

Detect leaked DockerHttpClient.Response objects #1421

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.

Sign up for GitHub

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

Merged
bsideup merged 1 commit into from
Jun 23, 2020
Merged

Detect leaked DockerHttpClient.Response objects #1421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions docker-java-core/src/main/java/com/github/dockerjava/core/DefaultInvocationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.apache.commons.io.IOUtils;

import java.io.ByteArrayInputStream;
import java.io.FilterInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -99,7 +100,17 @@ public InputStream post(Object entity) {
.body(encode(entity))
.build();

return execute(request).getBody();
DockerHttpClient.Response response = execute(request);
return new FilterInputStream(response.getBody()) {
@Override
public void close() throws IOException {
try {
super.close();
} finally {
response.close();
}
}
};
}

@Override
Expand Down Expand Up @@ -188,7 +199,17 @@ public InputStream get() {
.method(DockerHttpClient.Request.Method.GET)
.build();

return execute(request).getBody();
DockerHttpClient.Response response = execute(request);
return new FilterInputStream(response.getBody()) {
@Override
public void close() throws IOException {
try {
super.close();
} finally {
response.close();
}
}
};
}

@Override
Expand Down Expand Up @@ -244,8 +265,12 @@ protected <T> void executeAndStream(
Consumer<DockerHttpClient.Response> sourceConsumer
) {
Thread thread = new Thread(() -> {
Thread streamingThread = Thread.currentThread();
try (DockerHttpClient.Response response = execute(request)) {
callback.onStart(response);
callback.onStart(() -> {
streamingThread.interrupt();
response.close();
});

sourceConsumer.accept(response);
callback.onComplete();
Expand Down
8 changes: 7 additions & 1 deletion docker-java-core/src/main/java/com/github/dockerjava/core/exec/ResizeContainerCmdExec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;

public class ResizeContainerCmdExec extends AbstrSyncDockerCmdExec<ResizeContainerCmd, Void> implements ResizeContainerCmd.Exec {

private static final Logger LOGGER = LoggerFactory.getLogger(ResizeContainerCmdExec.class);
Expand All @@ -23,7 +25,11 @@ protected Void execute(ResizeContainerCmd command) {

LOGGER.trace("POST: {}", webResource);

webResource.request().accept(MediaType.APPLICATION_JSON).post(command);
try {
webResource.request().accept(MediaType.APPLICATION_JSON).post(command).close();
} catch (IOException e) {
throw new RuntimeException(e);
}

return null;
}
Expand Down
8 changes: 7 additions & 1 deletion docker-java-core/src/main/java/com/github/dockerjava/core/exec/ResizeExecCmdExec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;


public class ResizeExecCmdExec extends AbstrSyncDockerCmdExec<ResizeExecCmd, Void> implements ResizeExecCmd.Exec {

Expand All @@ -23,7 +25,11 @@ protected Void execute(ResizeExecCmd command) {

LOGGER.trace("POST: {}", webResource);

webResource.request().accept(MediaType.APPLICATION_JSON).post(null);
try {
webResource.request().accept(MediaType.APPLICATION_JSON).post(null).close();
} catch (IOException e) {
throw new RuntimeException(e);
}

return null;
}
Expand Down
13 changes: 10 additions & 3 deletions ...-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkDockerHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.github.dockerjava.transport.DockerHttpClient;
import com.github.dockerjava.transport.SSLConfig;
import okhttp3.Call;
import okhttp3.ConnectionPool;
import okhttp3.Dns;
import okhttp3.HttpUrl;
Expand Down Expand Up @@ -219,9 +220,11 @@ public Response execute(Request request) {
clientToUse = streamingClient;
}

Call call = clientToUse.newCall(requestBuilder.build());
try {
return new OkResponse(clientToUse.newCall(requestBuilder.build()).execute());
return new OkResponse(call);
} catch (IOException e) {
call.cancel();
throw new UncheckedIOException("Error while executing " + request, e);
}
}
Expand All @@ -239,10 +242,13 @@ static class OkResponse implements Response {

static final ThreadLocal<Boolean> CLOSING = ThreadLocal.withInitial(() -> false);

private final Call call;

private final okhttp3.Response response;

OkResponse(okhttp3.Response response) {
this.response = response;
OkResponse(Call call) throws IOException {
this.call = call;
this.response = call.execute();
}

@Override
Expand Down Expand Up @@ -270,6 +276,7 @@ public void close() {
boolean previous = CLOSING.get();
CLOSING.set(true);
try {
call.cancel();
response.close();
} catch (Exception | AssertionError e) {
LOGGER.debug("Failed to close the response", e);
Expand Down
36 changes: 22 additions & 14 deletions docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,13 @@ public DockerClientImpl createDockerClient(DockerClientConfig config) {
public DockerClientImpl createDockerClient(DockerClientConfig config) {
return (DockerClientImpl) DockerClientBuilder.getInstance(config)
.withDockerHttpClient(
new JerseyDockerHttpClient.Builder()
.dockerHost(config.getDockerHost())
.sslConfig(config.getSSLConfig())
.connectTimeout(30 * 1000)
.build()
new TrackingDockerHttpClient(
new JerseyDockerHttpClient.Builder()
.dockerHost(config.getDockerHost())
.sslConfig(config.getSSLConfig())
.connectTimeout(30 * 1000)
.build()
)
)
.build();
}
Expand All @@ -53,11 +55,13 @@ public DockerClientImpl createDockerClient(DockerClientConfig config) {
public DockerClientImpl createDockerClient(DockerClientConfig config) {
return (DockerClientImpl) DockerClientBuilder.getInstance(config)
.withDockerHttpClient(
new OkDockerHttpClient.Builder()
.dockerHost(config.getDockerHost())
.sslConfig(config.getSSLConfig())
.connectTimeout(30 * 100)
.build()
new TrackingDockerHttpClient(
new OkDockerHttpClient.Builder()
.dockerHost(config.getDockerHost())
.sslConfig(config.getSSLConfig())
.connectTimeout(30 * 100)
.build()
)
)
.build();
}
Expand All @@ -67,10 +71,12 @@ public DockerClientImpl createDockerClient(DockerClientConfig config) {
public DockerClientImpl createDockerClient(DockerClientConfig config) {
return (DockerClientImpl) DockerClientBuilder.getInstance(config)
.withDockerHttpClient(
new ApacheDockerHttpClient.Builder()
.dockerHost(config.getDockerHost())
.sslConfig(config.getSSLConfig())
.build()
new TrackingDockerHttpClient(
new ApacheDockerHttpClient.Builder()
.dockerHost(config.getDockerHost())
.sslConfig(config.getSSLConfig())
.build()
)
)
.build();
}
Expand Down Expand Up @@ -110,4 +116,6 @@ public FactoryType getFactoryType() {
@Rule
public DockerRule dockerRule = new DockerRule( this);

@Rule
public DockerHttpClientLeakDetector leakDetector = new DockerHttpClientLeakDetector();
}
31 changes: 31 additions & 0 deletions docker-java/src/test/java/com/github/dockerjava/cmd/DockerHttpClientLeakDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.github.dockerjava.cmd;

import org.junit.rules.ExternalResource;

public class DockerHttpClientLeakDetector extends ExternalResource {

@Override
protected void before() {
synchronized (TrackingDockerHttpClient.ACTIVE_RESPONSES) {
TrackingDockerHttpClient.ACTIVE_RESPONSES.clear();
}
}

@Override
protected void after() {
synchronized (TrackingDockerHttpClient.ACTIVE_RESPONSES) {
if (TrackingDockerHttpClient.ACTIVE_RESPONSES.isEmpty()) {
return;
}

System.out.println("Leaked responses:");
IllegalStateException exception = new IllegalStateException("Leaked responses!");
exception.setStackTrace(new StackTraceElement[0]);

TrackingDockerHttpClient.ACTIVE_RESPONSES.forEach(response -> {
exception.addSuppressed(response.allocatedAt);
});
throw exception;
}
}
}
20 changes: 13 additions & 7 deletions docker-java/src/test/java/com/github/dockerjava/cmd/SaveImageCmdIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,19 @@ public class SaveImageCmdIT extends CmdIT {
@Test
public void saveImage() throws Exception {

InputStream image = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImageCmd("busybox").exec());
assertThat(image.read(), not(-1));

InputStream image2 = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImageCmd("busybox").withTag("latest").exec());
assertThat(image2.read(), not(-1));


try (
InputStream inputStream = dockerRule.getClient().saveImageCmd("busybox").exec();
InputStream image = IOUtils.toBufferedInputStream(inputStream)
) {
assertThat(image.read(), not(-1));
}

try (
InputStream inputStream = dockerRule.getClient().saveImageCmd("busybox").withTag("latest").exec();
InputStream image2 = IOUtils.toBufferedInputStream(inputStream)
) {
assertThat(image2.read(), not(-1));
}
}

}
25 changes: 16 additions & 9 deletions docker-java/src/test/java/com/github/dockerjava/cmd/SaveImagesCmdIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,29 +15,36 @@ public class SaveImagesCmdIT extends CmdIT {

@Test
public void saveNoImages() throws Exception {
try(final InputStream image = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImagesCmd().exec())){
try (
InputStream inputStream = dockerRule.getClient().saveImagesCmd().exec();
InputStream image = IOUtils.toBufferedInputStream(inputStream)
){
assertThat(image.read(), not(-1));
}

}

@Test
public void saveImagesWithNameAndTag() throws Exception {

try(final InputStream image = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImagesCmd().withImage("busybox", "latest").exec())) {
try (
InputStream inputStream = dockerRule.getClient().saveImagesCmd().withImage("busybox", "latest").exec();
InputStream image = IOUtils.toBufferedInputStream(inputStream)
) {
assertThat(image.read(), not(-1));
}

}

@Test
public void saveMultipleImages() throws Exception {

try(final InputStream image = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImagesCmd()
// Not a real life use-case but "busybox" is the only one I dare to assume is really there.
.withImage("busybox", "latest")
.withImage("busybox", "latest")
.exec())) {
try (
InputStream inputStream = dockerRule.getClient().saveImagesCmd()
// Not a real life use-case but "busybox" is the only one I dare to assume is really there.
.withImage("busybox", "latest")
.withImage("busybox", "latest")
.exec();
InputStream image = IOUtils.toBufferedInputStream(inputStream)
) {
assertThat(image.read(), not(-1));
}
}
Expand Down
Loading