Skip to content

Add timeouts, cleanup #1336

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
KostyaSha merged 8 commits into from
Mar 7, 2020
Merged

Add timeouts, cleanup #1336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ce1fe66
Add timeouts, cleanup
KostyaSha Mar 3, 2020
ae312bc
Enlarge timeout
KostyaSha Mar 4, 2020
b04832d
restore value ffor tests
KostyaSha Mar 4, 2020
4dadc6f
Mac ttypos
KostyaSha Mar 4, 2020
5bfa433
Configurable options with default
KostyaSha Mar 4, 2020
10d8350
allign
KostyaSha Mar 4, 2020
f43118a
Comment
KostyaSha Mar 4, 2020
113a6f5
Unformat
KostyaSha Mar 4, 2020
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
19 changes: 19 additions & 0 deletions docker-java-core/src/main/java/com/github/dockerjava/core/AbstractDockerCmdExecFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ public abstract class AbstractDockerCmdExecFactory implements DockerCmdExecFacto

private DockerClientConfig dockerClientConfig;

protected Integer connectTimeout;
protected Integer readTimeout;
Copy link
Member

@bsideup bsideup Mar 4, 2020

Choose a reason for hiding this comment

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

readTimeout is wrong since there can be long living connections. The correct approach would be to use timeouts/deadlines when executing the commands

Copy link
Member Author

@KostyaSha KostyaSha Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For library that doesn't matter, you can set any timeouts on your app side.

Copy link
Member

@bsideup bsideup Mar 4, 2020

Choose a reason for hiding this comment

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

you should not be setting the default read timeout here. Some operations last minutes or even hours. The callee should set the timeouts at the use site


protected DockerClientConfig getDockerClientConfig() {
checkNotNull(dockerClientConfig,
"Factor not initialized, dockerClientConfig not set. You probably forgot to call init()!");
Expand All @@ -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());
Expand Down
14 changes: 0 additions & 14 deletions ...ransport-jersey/src/main/java/com/github/dockerjava/jaxrs/JerseyDockerCmdExecFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
27 changes: 4 additions & 23 deletions ...-transport-netty/src/main/java/com/github/dockerjava/netty/NettyDockerCmdExecFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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 {
Copy link
Member Author

@KostyaSha KostyaSha Mar 3, 2020

Choose a reason for hiding this comment

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

redundant implements

public class NettyDockerCmdExecFactory extends AbstractDockerCmdExecFactory {

private static String threadPrefix = "dockerjava-netty";

Expand Down Expand Up @@ -88,10 +89,6 @@ public DuplexChannel getChannel() {
}
};

private Integer connectTimeout = null;

private Integer readTimeout = null;

@Override
public void init(DockerClientConfig dockerClientConfig) {
super.init(dockerClientConfig);
Expand Down Expand Up @@ -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 extends Channel> 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());
}

Expand Down
47 changes: 33 additions & 14 deletions ...ansport-okhttp/src/main/java/com/github/dockerjava/okhttp/OkHttpDockerCmdExecFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,45 @@
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";

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);

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);
} else {
// default is too small for most docker commands, set default like in jersey/netty
clientBuilder.readTimeout(0, TimeUnit.MILLISECONDS);
}

if (nonNull(connectTimeout)) {
clientBuilder.connectTimeout(connectTimeout, TimeUnit.MILLISECONDS);
}

if (nonNull(retryOnConnectionFailure)) {
clientBuilder.retryOnConnectionFailure(retryOnConnectionFailure);
} else {
clientBuilder.retryOnConnectionFailure(true);
}

URI dockerHost = dockerClientConfig.getDockerHost();
switch (dockerHost.getScheme()) {
Expand All @@ -45,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
Expand All @@ -72,8 +92,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);
Expand All @@ -88,14 +107,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();
Expand Down
6 changes: 3 additions & 3 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 @@ -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();
return new OkHttpDockerCmdExecFactory().withConnectTimeout(30 * 1000);
}
};

Expand Down
15 changes: 9 additions & 6 deletions docker-java/src/test/java/com/github/dockerjava/cmd/StatsCmdIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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();

Expand All @@ -52,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");
Expand All @@ -67,7 +70,7 @@ public void testStatsNoStreaming() throws InterruptedException, IOException {
assertEquals("Expected stats called only once", countDownLatch.getCount(), NUM_STATS - 1);
}

private class StatsCallbackTest extends ResultCallbackTemplate<StatsCallbackTest, Statistics> {
private static class StatsCallbackTest extends ResultCallbackTemplate<StatsCallbackTest, Statistics> {
private final CountDownLatch countDownLatch;

private Boolean gotStats = false;
Expand Down