Skip to content

Commit dbc4761

Browse files
bsideupbsideup
authored
Detect leaked DockerHttpClient.Response objects (#1421)
1 parent 1730733 commit dbc4761

File tree

9 files changed

+225
-38
lines changed

9 files changed

+225
-38
lines changed

‎docker-java-core/src/main/java/com/github/dockerjava/core/DefaultInvocationBuilder.java‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.apache.commons.io.IOUtils;
1919

2020
import java.io.ByteArrayInputStream;
21+
import java.io.FilterInputStream;
2122
import java.io.IOException;
2223
import java.io.InputStream;
2324
import java.nio.charset.StandardCharsets;
@@ -99,7 +100,17 @@ public InputStream post(Object entity) {
99100
.body(encode(entity))
100101
.build();
101102

102-
return execute(request).getBody();
103+
DockerHttpClient.Response response = execute(request);
104+
return new FilterInputStream(response.getBody()) {
105+
@Override
106+
public void close() throws IOException {
107+
try {
108+
super.close();
109+
} finally {
110+
response.close();
111+
}
112+
}
113+
};
103114
}
104115

105116
@Override
@@ -188,7 +199,17 @@ public InputStream get() {
188199
.method(DockerHttpClient.Request.Method.GET)
189200
.build();
190201

191-
return execute(request).getBody();
202+
DockerHttpClient.Response response = execute(request);
203+
return new FilterInputStream(response.getBody()) {
204+
@Override
205+
public void close() throws IOException {
206+
try {
207+
super.close();
208+
} finally {
209+
response.close();
210+
}
211+
}
212+
};
192213
}
193214

194215
@Override
@@ -244,8 +265,12 @@ protected <T> void executeAndStream(
244265
Consumer<DockerHttpClient.Response> sourceConsumer
245266
) {
246267
Thread thread = new Thread(() -> {
268+
Thread streamingThread = Thread.currentThread();
247269
try (DockerHttpClient.Response response = execute(request)) {
248-
callback.onStart(response);
270+
callback.onStart(() -> {
271+
streamingThread.interrupt();
272+
response.close();
273+
});
249274

250275
sourceConsumer.accept(response);
251276
callback.onComplete();

‎docker-java-core/src/main/java/com/github/dockerjava/core/exec/ResizeContainerCmdExec.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
import org.slf4j.Logger;
88
import org.slf4j.LoggerFactory;
99

10+
import java.io.IOException;
11+
1012
public class ResizeContainerCmdExec extends AbstrSyncDockerCmdExec<ResizeContainerCmd, Void> implements ResizeContainerCmd.Exec {
1113

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

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

26-
webResource.request().accept(MediaType.APPLICATION_JSON).post(command);
28+
try {
29+
webResource.request().accept(MediaType.APPLICATION_JSON).post(command).close();
30+
} catch (IOException e) {
31+
throw new RuntimeException(e);
32+
}
2733

2834
return null;
2935
}

‎docker-java-core/src/main/java/com/github/dockerjava/core/exec/ResizeExecCmdExec.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
import org.slf4j.Logger;
88
import org.slf4j.LoggerFactory;
99

10+
import java.io.IOException;
11+
1012

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

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

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

26-
webResource.request().accept(MediaType.APPLICATION_JSON).post(null);
28+
try {
29+
webResource.request().accept(MediaType.APPLICATION_JSON).post(null).close();
30+
} catch (IOException e) {
31+
throw new RuntimeException(e);
32+
}
2733

2834
return null;
2935
}

‎docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkDockerHttpClient.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.github.dockerjava.transport.DockerHttpClient;
44
import com.github.dockerjava.transport.SSLConfig;
5+
import okhttp3.Call;
56
import okhttp3.ConnectionPool;
67
import okhttp3.Dns;
78
import okhttp3.HttpUrl;
@@ -219,9 +220,11 @@ public Response execute(Request request) {
219220
clientToUse = streamingClient;
220221
}
221222

223+
Call call = clientToUse.newCall(requestBuilder.build());
222224
try {
223-
return new OkResponse(clientToUse.newCall(requestBuilder.build()).execute());
225+
return new OkResponse(call);
224226
} catch (IOException e) {
227+
call.cancel();
225228
throw new UncheckedIOException("Error while executing " + request, e);
226229
}
227230
}
@@ -239,10 +242,13 @@ static class OkResponse implements Response {
239242

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

245+
private final Call call;
246+
242247
private final okhttp3.Response response;
243248

244-
OkResponse(okhttp3.Response response) {
245-
this.response = response;
249+
OkResponse(Call call) throws IOException {
250+
this.call = call;
251+
this.response = call.execute();
246252
}
247253

248254
@Override
@@ -270,6 +276,7 @@ public void close() {
270276
boolean previous = CLOSING.get();
271277
CLOSING.set(true);
272278
try {
279+
call.cancel();
273280
response.close();
274281
} catch (Exception | AssertionError e) {
275282
LOGGER.debug("Failed to close the response", e);

‎docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,13 @@ public DockerClientImpl createDockerClient(DockerClientConfig config) {
3939
public DockerClientImpl createDockerClient(DockerClientConfig config) {
4040
return (DockerClientImpl) DockerClientBuilder.getInstance(config)
4141
.withDockerHttpClient(
42-
new JerseyDockerHttpClient.Builder()
43-
.dockerHost(config.getDockerHost())
44-
.sslConfig(config.getSSLConfig())
45-
.connectTimeout(30 * 1000)
46-
.build()
42+
new TrackingDockerHttpClient(
43+
new JerseyDockerHttpClient.Builder()
44+
.dockerHost(config.getDockerHost())
45+
.sslConfig(config.getSSLConfig())
46+
.connectTimeout(30 * 1000)
47+
.build()
48+
)
4749
)
4850
.build();
4951
}
@@ -53,11 +55,13 @@ public DockerClientImpl createDockerClient(DockerClientConfig config) {
5355
public DockerClientImpl createDockerClient(DockerClientConfig config) {
5456
return (DockerClientImpl) DockerClientBuilder.getInstance(config)
5557
.withDockerHttpClient(
56-
new OkDockerHttpClient.Builder()
57-
.dockerHost(config.getDockerHost())
58-
.sslConfig(config.getSSLConfig())
59-
.connectTimeout(30 * 100)
60-
.build()
58+
new TrackingDockerHttpClient(
59+
new OkDockerHttpClient.Builder()
60+
.dockerHost(config.getDockerHost())
61+
.sslConfig(config.getSSLConfig())
62+
.connectTimeout(30 * 100)
63+
.build()
64+
)
6165
)
6266
.build();
6367
}
@@ -67,10 +71,12 @@ public DockerClientImpl createDockerClient(DockerClientConfig config) {
6771
public DockerClientImpl createDockerClient(DockerClientConfig config) {
6872
return (DockerClientImpl) DockerClientBuilder.getInstance(config)
6973
.withDockerHttpClient(
70-
new ApacheDockerHttpClient.Builder()
71-
.dockerHost(config.getDockerHost())
72-
.sslConfig(config.getSSLConfig())
73-
.build()
74+
new TrackingDockerHttpClient(
75+
new ApacheDockerHttpClient.Builder()
76+
.dockerHost(config.getDockerHost())
77+
.sslConfig(config.getSSLConfig())
78+
.build()
79+
)
7480
)
7581
.build();
7682
}
@@ -110,4 +116,6 @@ public FactoryType getFactoryType() {
110116
@Rule
111117
public DockerRule dockerRule = new DockerRule( this);
112118

119+
@Rule
120+
public DockerHttpClientLeakDetector leakDetector = new DockerHttpClientLeakDetector();
113121
}

‎docker-java/src/test/java/com/github/dockerjava/cmd/DockerHttpClientLeakDetector.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
package com.github.dockerjava.cmd;
2+
3+
import org.junit.rules.ExternalResource;
4+
5+
public class DockerHttpClientLeakDetector extends ExternalResource {
6+
7+
@Override
8+
protected void before() {
9+
synchronized (TrackingDockerHttpClient.ACTIVE_RESPONSES) {
10+
TrackingDockerHttpClient.ACTIVE_RESPONSES.clear();
11+
}
12+
}
13+
14+
@Override
15+
protected void after() {
16+
synchronized (TrackingDockerHttpClient.ACTIVE_RESPONSES) {
17+
if (TrackingDockerHttpClient.ACTIVE_RESPONSES.isEmpty()) {
18+
return;
19+
}
20+
21+
System.out.println("Leaked responses:");
22+
IllegalStateException exception = new IllegalStateException("Leaked responses!");
23+
exception.setStackTrace(new StackTraceElement[0]);
24+
25+
TrackingDockerHttpClient.ACTIVE_RESPONSES.forEach(response -> {
26+
exception.addSuppressed(response.allocatedAt);
27+
});
28+
throw exception;
29+
}
30+
}
31+
}

‎docker-java/src/test/java/com/github/dockerjava/cmd/SaveImageCmdIT.java‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,19 @@ public class SaveImageCmdIT extends CmdIT {
1616
@Test
1717
public void saveImage() throws Exception {
1818

19-
InputStream image = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImageCmd("busybox").exec());
20-
assertThat(image.read(), not(-1));
21-
22-
InputStream image2 = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImageCmd("busybox").withTag("latest").exec());
23-
assertThat(image2.read(), not(-1));
24-
25-
19+
try (
20+
InputStream inputStream = dockerRule.getClient().saveImageCmd("busybox").exec();
21+
InputStream image = IOUtils.toBufferedInputStream(inputStream)
22+
) {
23+
assertThat(image.read(), not(-1));
24+
}
25+
26+
try (
27+
InputStream inputStream = dockerRule.getClient().saveImageCmd("busybox").withTag("latest").exec();
28+
InputStream image2 = IOUtils.toBufferedInputStream(inputStream)
29+
) {
30+
assertThat(image2.read(), not(-1));
31+
}
2632
}
2733

2834
}

‎docker-java/src/test/java/com/github/dockerjava/cmd/SaveImagesCmdIT.java‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,29 +15,36 @@ public class SaveImagesCmdIT extends CmdIT {
1515

1616
@Test
1717
public void saveNoImages() throws Exception {
18-
try(final InputStream image = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImagesCmd().exec())){
18+
try (
19+
InputStream inputStream = dockerRule.getClient().saveImagesCmd().exec();
20+
InputStream image = IOUtils.toBufferedInputStream(inputStream)
21+
){
1922
assertThat(image.read(), not(-1));
2023
}
2124

2225
}
2326

2427
@Test
2528
public void saveImagesWithNameAndTag() throws Exception {
26-
27-
try(final InputStream image = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImagesCmd().withImage("busybox", "latest").exec())) {
29+
try (
30+
InputStream inputStream = dockerRule.getClient().saveImagesCmd().withImage("busybox", "latest").exec();
31+
InputStream image = IOUtils.toBufferedInputStream(inputStream)
32+
) {
2833
assertThat(image.read(), not(-1));
2934
}
3035

3136
}
3237

3338
@Test
3439
public void saveMultipleImages() throws Exception {
35-
36-
try(final InputStream image = IOUtils.toBufferedInputStream(dockerRule.getClient().saveImagesCmd()
37-
// Not a real life use-case but "busybox" is the only one I dare to assume is really there.
38-
.withImage("busybox", "latest")
39-
.withImage("busybox", "latest")
40-
.exec())) {
40+
try (
41+
InputStream inputStream = dockerRule.getClient().saveImagesCmd()
42+
// Not a real life use-case but "busybox" is the only one I dare to assume is really there.
43+
.withImage("busybox", "latest")
44+
.withImage("busybox", "latest")
45+
.exec();
46+
InputStream image = IOUtils.toBufferedInputStream(inputStream)
47+
) {
4148
assertThat(image.read(), not(-1));
4249
}
4350
}

0 commit comments

Comments
 (0)