From ce1fe66346b4837fc17975e41e90a4cbdd796f59 Mon Sep 17 00:00:00 2001 From: Kanstantsin Shautsou Date: Wed, 4 Mar 2020 01:31:04 +0300 Subject: [PATCH 1/8] Add timeouts, cleanup --- .../core/AbstractDockerCmdExecFactory.java | 19 +++++++++++++ .../jaxrs/JerseyDockerCmdExecFactory.java | 14 ---------- .../netty/NettyDockerCmdExecFactory.java | 27 +++---------------- .../okhttp/OkHttpDockerCmdExecFactory.java | 14 +++++++--- .../java/com/github/dockerjava/cmd/CmdIT.java | 2 +- 5 files changed, 35 insertions(+), 41 deletions(-) diff --git a/docker-java-core/src/main/java/com/github/dockerjava/core/AbstractDockerCmdExecFactory.java b/docker-java-core/src/main/java/com/github/dockerjava/core/AbstractDockerCmdExecFactory.java index 34fdceb37..7088b7277 100644 --- a/docker-java-core/src/main/java/com/github/dockerjava/core/AbstractDockerCmdExecFactory.java +++ b/docker-java-core/src/main/java/com/github/dockerjava/core/AbstractDockerCmdExecFactory.java @@ -150,6 +150,9 @@ public abstract class AbstractDockerCmdExecFactory implements DockerCmdExecFacto private DockerClientConfig dockerClientConfig; + protected Integer connectTimeout; + protected Integer readTimeout; + protected DockerClientConfig getDockerClientConfig() { checkNotNull(dockerClientConfig, "Factor not initialized, dockerClientConfig not set. You probably forgot to call init()!"); @@ -172,6 +175,22 @@ public CopyArchiveToContainerCmd.Exec createCopyArchiveToContainerCmdExec() { return new CopyArchiveToContainerCmdExec(getBaseResource(), getDockerClientConfig()); } + /** + * Configure connection timeout in milliseconds + */ + public AbstractDockerCmdExecFactory withConnectTimeout(Integer connectTimeout) { + this.connectTimeout = connectTimeout; + return this; + } + + /** + * Configure read timeout in milliseconds + */ + public AbstractDockerCmdExecFactory withReadTimeout(Integer readTimeout) { + this.readTimeout = readTimeout; + return this; + } + @Override public AuthCmd.Exec createAuthCmdExec() { return new AuthCmdExec(getBaseResource(), getDockerClientConfig()); diff --git a/docker-java-transport-jersey/src/main/java/com/github/dockerjava/jaxrs/JerseyDockerCmdExecFactory.java b/docker-java-transport-jersey/src/main/java/com/github/dockerjava/jaxrs/JerseyDockerCmdExecFactory.java index edd659678..97ec72384 100644 --- a/docker-java-transport-jersey/src/main/java/com/github/dockerjava/jaxrs/JerseyDockerCmdExecFactory.java +++ b/docker-java-transport-jersey/src/main/java/com/github/dockerjava/jaxrs/JerseyDockerCmdExecFactory.java @@ -50,10 +50,6 @@ public class JerseyDockerCmdExecFactory extends AbstractDockerCmdExecFactory { private JerseyWebTarget baseResource; - private Integer readTimeout = null; - - private Integer connectTimeout = null; - private Integer maxTotalConnections = null; private Integer maxPerRouteConnections = null; @@ -262,16 +258,6 @@ public void close() throws IOException { connManager.close(); } - public JerseyDockerCmdExecFactory withReadTimeout(Integer readTimeout) { - this.readTimeout = readTimeout; - return this; - } - - public JerseyDockerCmdExecFactory withConnectTimeout(Integer connectTimeout) { - this.connectTimeout = connectTimeout; - return this; - } - public JerseyDockerCmdExecFactory withMaxTotalConnections(Integer maxTotalConnections) { this.maxTotalConnections = maxTotalConnections; return this; diff --git a/docker-java-transport-netty/src/main/java/com/github/dockerjava/netty/NettyDockerCmdExecFactory.java b/docker-java-transport-netty/src/main/java/com/github/dockerjava/netty/NettyDockerCmdExecFactory.java index efc47a742..cc1c4452b 100644 --- a/docker-java-transport-netty/src/main/java/com/github/dockerjava/netty/NettyDockerCmdExecFactory.java +++ b/docker-java-transport-netty/src/main/java/com/github/dockerjava/netty/NettyDockerCmdExecFactory.java @@ -1,6 +1,7 @@ package com.github.dockerjava.netty; import static com.google.common.base.Preconditions.checkNotNull; +import static java.util.Objects.nonNull; import java.io.IOException; import java.net.InetAddress; @@ -58,7 +59,7 @@ * @see https://docs.docker.com/engine/reference/api/docker_remote_api_v1.21/#attach-to-a-container * @see https://docs.docker.com/engine/reference/api/docker_remote_api_v1.21/#exec-start */ -public class NettyDockerCmdExecFactory extends AbstractDockerCmdExecFactory implements DockerCmdExecFactory { +public class NettyDockerCmdExecFactory extends AbstractDockerCmdExecFactory { private static String threadPrefix = "dockerjava-netty"; @@ -88,10 +89,6 @@ public DuplexChannel getChannel() { } }; - private Integer connectTimeout = null; - - private Integer readTimeout = null; - @Override public void init(DockerClientConfig dockerClientConfig) { super.init(dockerClientConfig); @@ -292,29 +289,13 @@ public void close() throws IOException { eventLoopGroup.shutdownGracefully(); } - /** - * Configure connection timeout in milliseconds - */ - public NettyDockerCmdExecFactory withConnectTimeout(Integer connectTimeout) { - this.connectTimeout = connectTimeout; - return this; - } - - /** - * Configure read timeout in milliseconds - */ - public NettyDockerCmdExecFactory withReadTimeout(Integer readTimeout) { - this.readTimeout = readTimeout; - return this; - } - private T configure(T channel) { ChannelConfig channelConfig = channel.config(); - if (connectTimeout != null) { + if (nonNull(connectTimeout)) { channelConfig.setConnectTimeoutMillis(connectTimeout); } - if (readTimeout != null) { + if (nonNull(readTimeout)) { channel.pipeline().addLast("readTimeoutHandler", new ReadTimeoutHandler()); } diff --git a/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java b/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java index fb34bce7a..d4c5a502b 100644 --- a/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java +++ b/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java @@ -20,6 +20,8 @@ import java.util.Collections; import java.util.concurrent.TimeUnit; +import static java.util.Objects.nonNull; + public class OkHttpDockerCmdExecFactory extends AbstractDockerCmdExecFactory { private static final String SOCKET_SUFFIX = ".socket"; @@ -34,9 +36,15 @@ public class OkHttpDockerCmdExecFactory extends AbstractDockerCmdExecFactory { public void init(DockerClientConfig dockerClientConfig) { super.init(dockerClientConfig); - OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder() - .readTimeout(0, TimeUnit.SECONDS) - .retryOnConnectionFailure(true); + OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder(); + if (nonNull(readTimeout)) { + clientBuilder.readTimeout(readTimeout, TimeUnit.MILLISECONDS); + } + if (nonNull(connectTimeout)) { + clientBuilder.connectTimeout(connectTimeout, TimeUnit.MILLISECONDS); + } + + clientBuilder.retryOnConnectionFailure(true); URI dockerHost = dockerClientConfig.getDockerHost(); switch (dockerHost.getScheme()) { diff --git a/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java b/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java index e1784f0a9..3950f12bc 100644 --- a/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java +++ b/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java @@ -35,7 +35,7 @@ public DockerCmdExecFactory createExecFactory() { OKHTTP(true) { @Override public DockerCmdExecFactory createExecFactory() { - return new OkHttpDockerCmdExecFactory(); + return new OkHttpDockerCmdExecFactory().withConnectTimeout(10 * 1000); } }; From ae312bc8cea3d480e552a9e6855a51dc6f38181c Mon Sep 17 00:00:00 2001 From: Kanstantsin Shautsou Date: Wed, 4 Mar 2020 03:22:09 +0300 Subject: [PATCH 2/8] Enlarge timeout --- .../src/test/java/com/github/dockerjava/cmd/CmdIT.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java b/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java index 3950f12bc..215a552b6 100644 --- a/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java +++ b/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java @@ -23,19 +23,19 @@ public enum FactoryType { NETTY(true) { @Override public DockerCmdExecFactory createExecFactory() { - return new NettyDockerCmdExecFactory().withConnectTimeout(10 * 1000); + return new NettyDockerCmdExecFactory().withConnectTimeout(30 * 1000); } }, JERSEY(false) { @Override public DockerCmdExecFactory createExecFactory() { - return new JerseyDockerCmdExecFactory().withConnectTimeout(10 * 1000); + return new JerseyDockerCmdExecFactory().withConnectTimeout(30 * 1000); } }, OKHTTP(true) { @Override public DockerCmdExecFactory createExecFactory() { - return new OkHttpDockerCmdExecFactory().withConnectTimeout(10 * 1000); + return new OkHttpDockerCmdExecFactory().withConnectTimeout(30 * 1000); } }; From b04832d479a1535c6fe997226630adbc63eec778 Mon Sep 17 00:00:00 2001 From: Kanstantsin Shautsou Date: Wed, 4 Mar 2020 20:22:42 +0300 Subject: [PATCH 3/8] restore value ffor tests --- .../github/dockerjava/core/InvocationBuilder.java | 2 +- .../test/java/com/github/dockerjava/cmd/CmdIT.java | 4 +++- .../java/com/github/dockerjava/cmd/StatsCmdIT.java | 12 +++++++----- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/docker-java-core/src/main/java/com/github/dockerjava/core/InvocationBuilder.java b/docker-java-core/src/main/java/com/github/dockerjava/core/InvocationBuilder.java index 88b8707cf..7e81a9244 100644 --- a/docker-java-core/src/main/java/com/github/dockerjava/core/InvocationBuilder.java +++ b/docker-java-core/src/main/java/com/github/dockerjava/core/InvocationBuilder.java @@ -69,7 +69,7 @@ private void onResult(A_RES_T object) { } @Override - public void close() throws IOException { + public void close() throws IOException { try { super.close(); } finally { diff --git a/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java b/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java index 215a552b6..fab852ec6 100644 --- a/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java +++ b/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java @@ -35,7 +35,9 @@ public DockerCmdExecFactory createExecFactory() { OKHTTP(true) { @Override public DockerCmdExecFactory createExecFactory() { - return new OkHttpDockerCmdExecFactory().withConnectTimeout(30 * 1000); + return new OkHttpDockerCmdExecFactory() + .withConnectTimeout(60 * 1000) + .withReadTimeout(0); } }; diff --git a/docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java b/docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java index 377d4765c..f52d8d4d2 100644 --- a/docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java +++ b/docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java @@ -1,8 +1,8 @@ package com.github.dockerjava.cmd; +import com.github.dockerjava.api.async.ResultCallbackTemplate; import com.github.dockerjava.api.command.CreateContainerResponse; import com.github.dockerjava.api.model.Statistics; -import com.github.dockerjava.core.async.ResultCallbackTemplate; import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -28,8 +28,10 @@ public void testStatsStreaming() throws InterruptedException, IOException { dockerRule.getClient().startContainerCmd(container.getId()).exec(); boolean gotStats = false; - try (StatsCallbackTest statsCallback = dockerRule.getClient().statsCmd(container.getId()).exec( - new StatsCallbackTest(countDownLatch))) { + try (StatsCallbackTest statsCallback = dockerRule.getClient() + .statsCmd(container.getId()) + .exec(new StatsCallbackTest(countDownLatch))) { + assertTrue(countDownLatch.await(10, TimeUnit.SECONDS)); gotStats = statsCallback.gotStats(); @@ -53,7 +55,7 @@ public void testStatsNoStreaming() throws InterruptedException, IOException { dockerRule.getClient().startContainerCmd(container.getId()).exec(); try (StatsCallbackTest statsCallback = dockerRule.getClient().statsCmd(container.getId()).withNoStream(true).exec( - new StatsCallbackTest(countDownLatch))) { + new StatsCallbackTest(countDownLatch))) { countDownLatch.await(5, TimeUnit.SECONDS); LOG.info("Stop stats collection"); @@ -67,7 +69,7 @@ public void testStatsNoStreaming() throws InterruptedException, IOException { assertEquals("Expected stats called only once", countDownLatch.getCount(), NUM_STATS - 1); } - private class StatsCallbackTest extends ResultCallbackTemplate { + private static class StatsCallbackTest extends ResultCallbackTemplate { private final CountDownLatch countDownLatch; private Boolean gotStats = false; From 4dadc6f1d8ed9e6fc4b0dc31599addf02e4ff351 Mon Sep 17 00:00:00 2001 From: Kanstantsin Shautsou Date: Wed, 4 Mar 2020 20:29:03 +0300 Subject: [PATCH 4/8] Mac ttypos --- .../java/com/github/dockerjava/core/InvocationBuilder.java | 2 +- .../src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docker-java-core/src/main/java/com/github/dockerjava/core/InvocationBuilder.java b/docker-java-core/src/main/java/com/github/dockerjava/core/InvocationBuilder.java index 7e81a9244..88b8707cf 100644 --- a/docker-java-core/src/main/java/com/github/dockerjava/core/InvocationBuilder.java +++ b/docker-java-core/src/main/java/com/github/dockerjava/core/InvocationBuilder.java @@ -69,7 +69,7 @@ private void onResult(A_RES_T object) { } @Override - public void close() throws IOException { + public void close() throws IOException { try { super.close(); } finally { diff --git a/docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java b/docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java index f52d8d4d2..c4f9fef57 100644 --- a/docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java +++ b/docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java @@ -54,8 +54,9 @@ public void testStatsNoStreaming() throws InterruptedException, IOException { dockerRule.getClient().startContainerCmd(container.getId()).exec(); - try (StatsCallbackTest statsCallback = dockerRule.getClient().statsCmd(container.getId()).withNoStream(true).exec( - new StatsCallbackTest(countDownLatch))) { + try (StatsCallbackTest statsCallback = dockerRule.getClient().statsCmd(container.getId()) + .withNoStream(true) + .exec(new StatsCallbackTest(countDownLatch))) { countDownLatch.await(5, TimeUnit.SECONDS); LOG.info("Stop stats collection"); From 5bfa4336a76b3e713d42eac44a46d5977f5485d2 Mon Sep 17 00:00:00 2001 From: Kanstantsin Shautsou Date: Wed, 4 Mar 2020 20:58:52 +0300 Subject: [PATCH 5/8] Configurable options with default --- .../okhttp/OkHttpDockerCmdExecFactory.java | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java b/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java index d4c5a502b..eac40153d 100644 --- a/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java +++ b/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java @@ -29,9 +29,15 @@ public class OkHttpDockerCmdExecFactory extends AbstractDockerCmdExecFactory { private ObjectMapper objectMapper; private OkHttpClient okHttpClient; + private Boolean retryOnConnectionFailure; private HttpUrl baseUrl; + public OkHttpDockerCmdExecFactory setRetryOnConnectionFailure(Boolean retryOnConnectionFailure) { + this.retryOnConnectionFailure = retryOnConnectionFailure; + return this; + } + @Override public void init(DockerClientConfig dockerClientConfig) { super.init(dockerClientConfig); @@ -39,12 +45,19 @@ public void init(DockerClientConfig dockerClientConfig) { OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder(); if (nonNull(readTimeout)) { clientBuilder.readTimeout(readTimeout, TimeUnit.MILLISECONDS); + } else { + clientBuilder.readTimeout(0, TimeUnit.MILLISECONDS); } + if (nonNull(connectTimeout)) { clientBuilder.connectTimeout(connectTimeout, TimeUnit.MILLISECONDS); } - clientBuilder.retryOnConnectionFailure(true); + if (nonNull(retryOnConnectionFailure)) { + clientBuilder.retryOnConnectionFailure(retryOnConnectionFailure); + } else { + clientBuilder.retryOnConnectionFailure(true); + } URI dockerHost = dockerClientConfig.getDockerHost(); switch (dockerHost.getScheme()) { @@ -80,8 +93,7 @@ public void init(DockerClientConfig dockerClientConfig) { SSLContext sslContext = sslConfig.getSSLContext(); if (sslContext != null) { isSSL = true; - clientBuilder - .sslSocketFactory(sslContext.getSocketFactory(), new TrustAllX509TrustManager()); + clientBuilder.sslSocketFactory(sslContext.getSocketFactory(), new TrustAllX509TrustManager()); } } catch (Exception e) { throw new RuntimeException(e); @@ -96,14 +108,14 @@ public void init(DockerClientConfig dockerClientConfig) { case "unix": case "npipe": baseUrlBuilder = new HttpUrl.Builder() - .scheme("http") - .host("docker" + SOCKET_SUFFIX); + .scheme("http") + .host("docker" + SOCKET_SUFFIX); break; case "tcp": baseUrlBuilder = new HttpUrl.Builder() - .scheme(isSSL ? "https" : "http") - .host(dockerHost.getHost()) - .port(dockerHost.getPort()); + .scheme(isSSL ? "https" : "http") + .host(dockerHost.getHost()) + .port(dockerHost.getPort()); break; default: baseUrlBuilder = HttpUrl.get(dockerHost.toString()).newBuilder(); From 10d835068f1cf954c6225d3970fd48ee132bf822 Mon Sep 17 00:00:00 2001 From: Kanstantsin Shautsou Date: Wed, 4 Mar 2020 21:02:10 +0300 Subject: [PATCH 6/8] allign --- .../src/test/java/com/github/dockerjava/cmd/CmdIT.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java b/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java index fab852ec6..215a552b6 100644 --- a/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java +++ b/docker-java/src/test/java/com/github/dockerjava/cmd/CmdIT.java @@ -35,9 +35,7 @@ public DockerCmdExecFactory createExecFactory() { OKHTTP(true) { @Override public DockerCmdExecFactory createExecFactory() { - return new OkHttpDockerCmdExecFactory() - .withConnectTimeout(60 * 1000) - .withReadTimeout(0); + return new OkHttpDockerCmdExecFactory().withConnectTimeout(30 * 1000); } }; From f43118a031d900c0ae7863046ba2af8fdf1a808c Mon Sep 17 00:00:00 2001 From: Kanstantsin Shautsou Date: Wed, 4 Mar 2020 21:04:03 +0300 Subject: [PATCH 7/8] Comment --- .../com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java | 1 + 1 file changed, 1 insertion(+) diff --git a/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java b/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java index eac40153d..cb9526e02 100644 --- a/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java +++ b/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java @@ -46,6 +46,7 @@ public void init(DockerClientConfig dockerClientConfig) { if (nonNull(readTimeout)) { clientBuilder.readTimeout(readTimeout, TimeUnit.MILLISECONDS); } else { + // default is too small for most docker commands, set default like in jersey/netty clientBuilder.readTimeout(0, TimeUnit.MILLISECONDS); } From 113a6f5083a35b0b90ed3b693af320ba8830ddb3 Mon Sep 17 00:00:00 2001 From: Kanstantsin Shautsou Date: Wed, 4 Mar 2020 21:11:05 +0300 Subject: [PATCH 8/8] Unformat --- .../dockerjava/okhttp/OkHttpDockerCmdExecFactory.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java b/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java index cb9526e02..7b0030e40 100644 --- a/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java +++ b/docker-java-transport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java @@ -67,11 +67,9 @@ public void init(DockerClientConfig dockerClientConfig) { String socketPath = dockerHost.getPath(); if ("unix".equals(dockerHost.getScheme())) { - clientBuilder - .socketFactory(new UnixSocketFactory(socketPath)); + clientBuilder.socketFactory(new UnixSocketFactory(socketPath)); } else { - clientBuilder - .socketFactory(new NamedPipeSocketFactory(socketPath)); + clientBuilder.socketFactory(new NamedPipeSocketFactory(socketPath)); } clientBuilder