Repository navigation
readTimeout connectTimeout for netty? #588
Description
@marcuslinke did i miss something?
Reacted by Tomasz Szuba
| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
@marcuslinke did i miss something?
| Back | FazBrowse Home | New Git URL |
Activity
Netty is missing it?
marcuslinke commented on May 30, 2016
There are no generic timeouts implemented yet for netty factory. Is it a problem?
KostyaSha commented on May 30, 2016
no, continue comparing. It strange that timeouts are not configurable.
marcuslinke commented on May 30, 2016
Timeouts can be implemented explicitely by insering a special timeout handler into the channel pipeline.
KostyaSha commented on May 30, 2016
@marcuslinke how get SSLContext from already created connection or factory?
Use case, building connection with defaults that reading env vars and then want get created sslconfig/context from execfactory.
KostyaSha commented on May 30, 2016
Hm... and DockerClientBuilder is not usable if i'm not using dockerCertPath because .build() calls DockerClientConfig constructor that checks checkDockerCertPath
Migrate to docker-java-3 with netty
marcuslinke commented on May 31, 2016
Maybe we have to introduce DockerCmdExecFactory.getSSLContext() then?
Use public release version of TCP-Unix socket proxy
KostyaSha commented on Jun 6, 2016
#591 but i think my main issue is DockerClientConfigBuilder that wants dockerCertPath and doesn't know that SSL could be set in DockerCmdExec because it 2 different classes.
Migrate to docker-java-3 with netty (#30)
Refactor to use the docker-java v3 with Netty support
Refactor to use the docker-java v3 with Netty support
KostyaSha commented on Jul 12, 2016
And, without readtimeout/connecttimeout client may cause infinite delay, or it bug somewhere. In any case it would make also sense to add them in test clients.
pjdarton commented on Jan 15, 2018
Hi folks,
I just found this issue while searching for support for setting a readTimeout when using Netty. The changelog suggests that this functionality was lost when v2.0.0 arrived (it was implemented for JerseyDockerCmdExecFactory but not for NettyDockerCmdExecFactory).
I think that this missing functionality might be fairly trivial to implement if we can "just" add a few lines to JerseyDockerCmdExecFactory...
If my understanding of the Netty docs for ReadTimeoutHandler is correct, all we need to do is to add a new field to NettyDockerCmdExecFactory
add a new method
/** * Configure read timeout in milliseconds */ public NettyDockerCmdExecFactory withReadTimeout(Integer readTimeout) { this.readTimeout = readTimeout; return this; }and lastly modify the configure method so that this new field gets taken into account:
private <T extends Channel> T configure(T channel) { ChannelConfig channelConfig = channel.config(); if (connectTimeout != null) { channelConfig.setConnectTimeoutMillis(connectTimeout); } if (readTimeout != null) { channel.pipeline().addLast("readTimeoutHandler", new ReadTimeoutHandler(readTimeout, TimeUnit.MILLISECONDS)); } return channel; }As @KostyaSha said over a year ago, failure to set a timeout may cause an infinite delay - we have found this to be the case, and we get reminders of this annoyingly often.
In our environment (multiple docker hosts, Jenkins and a docker-plugin for Jenkins), what we've found is that sometimes the Docker daemon fails in a manner than means that it accepts new TCP/IP connections, but it never answers any API requests so, unless you've got some form of timeout, any call to docker-java can hang indefinitely. In my case, this was happening when Jenkins called the docker-plugin's provision method to ask for a new container to be created, and when docker-java locked up, most of Jenkins stopped working (which is why I noticed).
i.e. timeouts are important :-)
We don't want to reconfigure the Jenkins docker-plugin to use Jersey (too many library clashes with the core Jenkins code - we want to stay using Netty) but we really need to ensure that Jenkins gives up on a docker daemon if it fails to respond within 1 minute.
...so, what do you think? Is this likely to work? If so, what's the quickest way of making it happen?
KostyaSha commented on Jan 15, 2018
No problem in https://github.com/KostyaSha/yet-another-docker-plugin any other issues (like may others) is just docker-plugin issues that i will be ignoring.
pjdarton commented on Jan 15, 2018
Yes, I remember the split :-(
FYI docker-plugin now has a new maintainer, and they seem far more cooperative, so it may be possible to re-join the two implementations. Personally, I think it would be nice if there was just one the one docker plugin that gives end users the best of both worlds, but I appreciate that there would be a non-trivial amount of effort required to achieve this now that the two repositories have diverged...
I checked the code in your ClientBuilderForConnector.java and, unless I'm much mistaken, it looks like it only implements timeouts (both readTimeout and connectiTimeout) if the connector type is Jersey, and silently ignores it otherwise.
However, this (docker-java) isn't the place to be discussing Jenkins docker plugins.
Irrespective of Jenkins docker-plugin implementations, from a docker-java user's point of view, it would be much better if both readTimeout and connectionTimeout was implemented for both Jersey and Netty. At present, the docker-java API is unbalanced, with critical functionality remaining half-implemented due to its absence on the Netty side of things.
If we can enhance docker-java so that both timeouts are implemented for both Netty and Jersey then all docker-java users will benefit, including both docker-plugin and yet-another-docker-plugin.
KostyaSha commented on Jan 15, 2018
@pjdarton maintaining is happening on the same quality level, i had enough issues.
I remember about timeouts and collected different production issues, planning to enhance. In limited time scopes now i only review/merge PRs
pjdarton commented on Jan 22, 2018
I've implemented a method readTimeout on the Netty side of things.
I've tested this manually with a local build of docker-plugin while issuing a kill -s STOP <dockerPID> on my local docker host and it works as expected - Manage Jenkins -> Configure System -> Add cloud -> Test Connection can (if you configure a read timeout) now give up waiting for a response after the configured time (despite that plugin using Netty).
Merging this PR will then allow both docker-plugin and yet-another-docker-plugin (once enhanced to take advantage of this new functionality) to be able to implement both a "Connection Timeout" and a "Read Timeout" regardless of them using Netty or Jersey.