You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by GitBox <gi...@apache.org> on 2021/03/18 20:26:16 UTC
[GitHub] [httpcomponents-client] ok2c opened a new pull request #296: Connection management configuration
ok2c opened a new pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296
@rschmitt @carterkozak @michael-o @garydgregory I would like to propose the following changes to HttpClient 5.x configuration APIs. In essence all configuration parameters related to connection management and initialization have been moved from `RequestConfig` to a new config class.
This is just a draft at this point. You are welcome to propose alternative ideas, better class names and so on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r598124959
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
Review comment:
@ok2c I think the challenge here is that the connect timeout should actually be or additionally be on a per-route basis because the conn manager handles connections to different servers w/o different attributes in contrast to a database conn pool which in most cases connects to a single host. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-806484988
Going through...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601693927
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
@carterkozak To me, this is clearly an HTTP protocol layer responsibility, but if you, guys, are absolutely convinced I can move this setting to `ConnectionConfig`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c merged pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c merged pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601336300
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
>
>
> Another stupid question: `DEFAULT_CONN_KEEP_ALIVE` applies to the lifetime of the connection. Why is is retained here and not moved to the new config class?
The `keep-alive` period is determined on a per request basis, generally based on the `Keep-Alive` header value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602057662
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -243,6 +245,16 @@ private InternalConnectionEndpoint cast(final ConnectionEndpoint endpoint) {
throw new IllegalStateException("Unexpected endpoint class: " + endpoint.getClass());
}
+ private ConnectionConfig resolveConnectionConfig(final HttpRoute route) {
+ final ConnectionConfig connectionConfig = connectionConfigResolver != null ? connectionConfigResolver.resolve(route) : null;
+ return connectionConfig != null ? connectionConfig : ConnectionConfig.DEFAULT;
+ }
+
+ private SocketConfig resolveSocketConfig(final HttpRoute route) {
+ final SocketConfig socketConfig = socketConfigResolver != null ? socketConfigResolver.resolve(route) : null;
+ return socketConfig != null ? socketConfig : SocketConfig.DEFAULT;
Review comment:
@carterkozak Corrected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] rschmitt commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
rschmitt commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-803086069
It seems that `RequestConfig` and `ConnectionMgmtConfig` are always passed together and both specify per-request options. In that case, why not declare `ConnectionMgmtConfig` _on_ `RequestConfig`? In addition to simplifying the diff, it will make it clear that `ConnectionMgmtConfig` are all request-scoped (so to speak).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602084169
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java
##########
@@ -179,6 +181,20 @@ public synchronized void setSocketConfig(final SocketConfig socketConfig) {
this.socketConfig = socketConfig != null ? socketConfig : SocketConfig.DEFAULT;
}
+ /**
+ * @since 5.2
+ */
+ public synchronized ConnectionConfig getConnectionMgmtConfig() {
Review comment:
Mgmt leftover?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java
##########
@@ -179,6 +181,20 @@ public synchronized void setSocketConfig(final SocketConfig socketConfig) {
this.socketConfig = socketConfig != null ? socketConfig : SocketConfig.DEFAULT;
}
+ /**
+ * @since 5.2
+ */
+ public synchronized ConnectionConfig getConnectionMgmtConfig() {
+ return connectionConfig;
+ }
+
+ /**
+ * @since 5.2
+ */
+ public synchronized void setConnectionMgmtConfig(final ConnectionConfig connectionConfig) {
Review comment:
Mgmt leftover?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-808398380
>
>
> > Did you push? I still can find a lot of Mgmt in the changeset.
>
> @michael-o I overlooked variable names. Corrected.
>
> Do you want me to move `ConnectionRequestTimeout` to `ConnectionConfig`? I can do that but I suppose at some point we will have to deal with a bunch of angry people demanding those properties to be un-deprecated. `ConnectTimeout` would be a easier to argue about. `ConnectionRequestTimeout` is less so.
No, we should discuss this separately. It is basically a `connectionLeaseTimeout` from a connection manage. I am fine with the rest of the code....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-806554507
@carterkozak @michael-o Would you like `ConnectionConfig` better than `ConnectionMgmtConfig`? `ConnectionManagementConfig` is just too unwieldy in my opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602089409
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
Review comment:
Yet another stupid question: Since this is the time how long the pool at most should require to return a working connection, why is this a HTTP request level config?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-803290067
> Addtionally to the comments: I'd like to understand the followinig:
> Why has the proxy host has been moved to connection mgmt? In my understanding an HTTP proxy does not operate on socket level, but on protocol level. This would make sense if we'd use SOCKS5. Can you explain?
HTTP proxy do essentially operate on the socket level once a tunnel has been established through it. However, I will deprecate proxy settings in `RequestConfig` in favor of `HttpRoutePlanner`. People should be using `HttpRoutePlanner` for custom routing strategies anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602112229
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalH2ConnPool.java
##########
@@ -0,0 +1,102 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.async;
+
+import org.apache.hc.client5.http.config.ConnectionConfig;
+import org.apache.hc.core5.concurrent.CallbackContribution;
+import org.apache.hc.core5.concurrent.FutureCallback;
+import org.apache.hc.core5.function.Resolver;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.nio.ssl.TlsStrategy;
+import org.apache.hc.core5.http2.nio.pool.H2ConnPool;
+import org.apache.hc.core5.io.CloseMode;
+import org.apache.hc.core5.io.ModalCloseable;
+import org.apache.hc.core5.reactor.ConnectionInitiator;
+import org.apache.hc.core5.reactor.IOSession;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.hc.core5.util.Timeout;
+
+import java.net.InetSocketAddress;
+import java.util.concurrent.Future;
+
+class InternalH2ConnPool implements ModalCloseable {
+
+ private final H2ConnPool connPool;
+
+ private volatile Resolver<HttpHost, ConnectionConfig> connectionConfigResolver;
+
+ InternalH2ConnPool(final ConnectionInitiator connectionInitiator,
+ final Resolver<HttpHost, InetSocketAddress> addressResolver,
+ final TlsStrategy tlsStrategy) {
+ this.connPool = new H2ConnPool(connectionInitiator, addressResolver, tlsStrategy);
+ }
+
+ public void close(final CloseMode closeMode) {
+ connPool.close(closeMode);
+ }
+
+ public void close() {
+ connPool.close();
+ }
+
+ private ConnectionConfig resolveConnectionConfig(final HttpHost httpHost) {
+ final Resolver<HttpHost, ConnectionConfig> resolver = this.connectionConfigResolver;
+ final ConnectionConfig connectionConfig = resolver != null ? resolver.resolve(httpHost) : null;
+ return connectionConfig != null ? connectionConfig : ConnectionConfig.DEFAULT;
+ }
+
+ public Future<IOSession> getSession(
+ final HttpHost endpoint,
+ final Timeout connectTimeout,
+ final FutureCallback<IOSession> callback) {
+ final ConnectionConfig connMgmtConfig = resolveConnectionConfig(endpoint);
Review comment:
yet another one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601693927
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
@carterkozak To me, this is clearly an HTTP protocol layer responsibility, but if you, guys, are absolutely convinced I can move this setting to `ConnectionConfig`. The connection management layer is in control of TTL (total time to live), which is a slightly different thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r597967717
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
+ }
+
+ /**
+ * @see Builder#setProxy(HttpHost)
+ */
+ public HttpHost getProxy() {
+ return proxy;
+ }
+
+ /**
+ * @see Builder#setSocketTimeout(Timeout)
+ */
+ public Timeout getSocketTimeout() {
+ return socketTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectionRequestTimeout(Timeout)
+ */
+ public Timeout getConnectionRequestTimeout() {
+ return connectionRequestTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectTimeout(Timeout)
+ */
+ public Timeout getConnectTimeout() {
+ return connectTimeout;
+ }
+
+ @Override
+ protected ConnectionMgmtConfig clone() throws CloneNotSupportedException {
+ return (ConnectionMgmtConfig) super.clone();
+ }
+
+ @Override
+ public String toString() {
+ final StringBuilder builder = new StringBuilder();
+ builder.append("[");
+ builder.append(", proxy=").append(proxy);
+ builder.append(", connectionRequestTimeout=").append(connectionRequestTimeout);
+ builder.append(", connectTimeout=").append(connectTimeout);
+ builder.append("]");
Review comment:
Where did the socket timeout go?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
Review comment:
Does this really make sense to block for 3 min to get a connection from the pool?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -42,8 +42,6 @@
@Contract(threading = ThreadingBehavior.IMMUTABLE)
public class RequestConfig implements Cloneable {
- private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
Stupid question: Isn't that rather a connection pool thing and not a request config thing?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
+ }
+
+ /**
+ * @see Builder#setProxy(HttpHost)
+ */
+ public HttpHost getProxy() {
+ return proxy;
+ }
+
+ /**
+ * @see Builder#setSocketTimeout(Timeout)
+ */
+ public Timeout getSocketTimeout() {
+ return socketTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectionRequestTimeout(Timeout)
+ */
+ public Timeout getConnectionRequestTimeout() {
+ return connectionRequestTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectTimeout(Timeout)
+ */
+ public Timeout getConnectTimeout() {
+ return connectTimeout;
+ }
+
+ @Override
+ protected ConnectionMgmtConfig clone() throws CloneNotSupportedException {
+ return (ConnectionMgmtConfig) super.clone();
+ }
+
+ @Override
+ public String toString() {
+ final StringBuilder builder = new StringBuilder();
+ builder.append("[");
+ builder.append(", proxy=").append(proxy);
+ builder.append(", connectionRequestTimeout=").append(connectionRequestTimeout);
+ builder.append(", connectTimeout=").append(connectTimeout);
+ builder.append("]");
+ return builder.toString();
+ }
+
+ public static ConnectionMgmtConfig.Builder custom() {
+ return new Builder();
+ }
+
+ public static ConnectionMgmtConfig.Builder copy(final ConnectionMgmtConfig config) {
+ return new Builder()
+ .setProxy(config.getProxy())
+ .setConnectionRequestTimeout(config.getConnectionRequestTimeout())
+ .setConnectTimeout(config.getConnectTimeout());
+ }
+
+ public static class Builder {
+
+ private HttpHost proxy;
+ private Timeout socketTimeout;
+ private Timeout connectionRequestTimeout;
+ private Timeout connectTimeout;
+
+ Builder() {
+ super();
+ this.connectionRequestTimeout = DEFAULT_CONNECTION_REQUEST_TIMEOUT;
+ this.connectTimeout = DEFAULT_CONNECT_TIMEOUT;
+ }
+
+
+ /**
+ * @see #setSocketTimeout(Timeout)
+ */
+ public Builder setSocketTimeout(final int soTimeout, final TimeUnit timeUnit) {
+ this.socketTimeout = Timeout.of(soTimeout, timeUnit);
+ return this;
+ }
+
+ /**
+ * Determines the default socket timeout value for I/O operations.
+ * <p>
+ * Default: 3 minutes
+ * </p>
+ *
+ * @return the default socket timeout value for I/O operations.
+ */
+ public Builder setSocketTimeout(final Timeout soTimeout) {
+ this.socketTimeout = soTimeout;
+ return this;
+ }
+
+ /**
+ * Returns HTTP proxy to be used for request execution.
+ * <p>
+ * Default: {@code null}
+ * </p>
+ */
+ public Builder setProxy(final HttpHost proxy) {
Review comment:
I obviously can't disable proxy if a gobal proxy has been set?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
+ }
+
+ /**
+ * @see Builder#setProxy(HttpHost)
+ */
+ public HttpHost getProxy() {
+ return proxy;
+ }
+
+ /**
+ * @see Builder#setSocketTimeout(Timeout)
+ */
+ public Timeout getSocketTimeout() {
+ return socketTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectionRequestTimeout(Timeout)
+ */
+ public Timeout getConnectionRequestTimeout() {
+ return connectionRequestTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectTimeout(Timeout)
+ */
+ public Timeout getConnectTimeout() {
+ return connectTimeout;
+ }
+
+ @Override
+ protected ConnectionMgmtConfig clone() throws CloneNotSupportedException {
+ return (ConnectionMgmtConfig) super.clone();
+ }
+
+ @Override
+ public String toString() {
+ final StringBuilder builder = new StringBuilder();
+ builder.append("[");
+ builder.append(", proxy=").append(proxy);
+ builder.append(", connectionRequestTimeout=").append(connectionRequestTimeout);
+ builder.append(", connectTimeout=").append(connectTimeout);
+ builder.append("]");
+ return builder.toString();
+ }
+
+ public static ConnectionMgmtConfig.Builder custom() {
+ return new Builder();
+ }
+
+ public static ConnectionMgmtConfig.Builder copy(final ConnectionMgmtConfig config) {
+ return new Builder()
+ .setProxy(config.getProxy())
+ .setConnectionRequestTimeout(config.getConnectionRequestTimeout())
+ .setConnectTimeout(config.getConnectTimeout());
Review comment:
Where did the socket timeout go?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
+ }
+
+ /**
+ * @see Builder#setProxy(HttpHost)
+ */
+ public HttpHost getProxy() {
+ return proxy;
+ }
+
+ /**
+ * @see Builder#setSocketTimeout(Timeout)
+ */
+ public Timeout getSocketTimeout() {
+ return socketTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectionRequestTimeout(Timeout)
+ */
+ public Timeout getConnectionRequestTimeout() {
+ return connectionRequestTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectTimeout(Timeout)
+ */
+ public Timeout getConnectTimeout() {
+ return connectTimeout;
+ }
+
+ @Override
+ protected ConnectionMgmtConfig clone() throws CloneNotSupportedException {
+ return (ConnectionMgmtConfig) super.clone();
+ }
+
+ @Override
+ public String toString() {
+ final StringBuilder builder = new StringBuilder();
+ builder.append("[");
+ builder.append(", proxy=").append(proxy);
+ builder.append(", connectionRequestTimeout=").append(connectionRequestTimeout);
+ builder.append(", connectTimeout=").append(connectTimeout);
+ builder.append("]");
+ return builder.toString();
+ }
+
+ public static ConnectionMgmtConfig.Builder custom() {
+ return new Builder();
+ }
+
+ public static ConnectionMgmtConfig.Builder copy(final ConnectionMgmtConfig config) {
+ return new Builder()
+ .setProxy(config.getProxy())
+ .setConnectionRequestTimeout(config.getConnectionRequestTimeout())
+ .setConnectTimeout(config.getConnectTimeout());
+ }
+
+ public static class Builder {
+
+ private HttpHost proxy;
+ private Timeout socketTimeout;
+ private Timeout connectionRequestTimeout;
+ private Timeout connectTimeout;
+
+ Builder() {
+ super();
+ this.connectionRequestTimeout = DEFAULT_CONNECTION_REQUEST_TIMEOUT;
+ this.connectTimeout = DEFAULT_CONNECT_TIMEOUT;
+ }
+
+
+ /**
+ * @see #setSocketTimeout(Timeout)
+ */
+ public Builder setSocketTimeout(final int soTimeout, final TimeUnit timeUnit) {
+ this.socketTimeout = Timeout.of(soTimeout, timeUnit);
+ return this;
+ }
+
+ /**
+ * Determines the default socket timeout value for I/O operations.
+ * <p>
+ * Default: 3 minutes
Review comment:
Wait, this is never set?!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601600940
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
So the overall idle time can change over time depending on this or the header value?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c edited a comment on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c edited a comment on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-805273488
@rschmitt @carterkozak @michael-o
Here's the second take:
* new `ConnectionMgmtConfig` structure configurable on the connection manager level
* proxy setting in `RequestConfig` deprecated in favor of `HttpRoutePlanner`
* connect timeout setting in `RequestConfig` deprecated in favor of `ConnectionMgmtConfig`
* `validateAfterInactivity` setting deprecated in favor `ConnectionMgmtConfig`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-802301299
Massive, I will need some time to digest. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-806571228
>
>
> @carterkozak @michael-o Would you like `ConnectionConfig` better than `ConnectionMgmtConfig`? `ConnectionManagementConfig` is just too unwieldy in my opinion.
Since we have `RequestConfig` and `SocketConfig` it would feel at least consistent to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601322389
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
Another stupid question: `DEFAULT_CONN_KEEP_ALIVE` applies to the lifetime of the connection. Why is is retained here and not moved to the new config class?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,202 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final Timeout socketTimeout;
+ private final Timeout connectTimeout;
+ private final TimeValue validateAfterInactivity;
Review comment:
Can you sort them like in the constructor, please?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
+ }
+
+ /**
+ * @see Builder#setProxy(HttpHost)
+ */
+ public HttpHost getProxy() {
+ return proxy;
+ }
+
+ /**
+ * @see Builder#setSocketTimeout(Timeout)
+ */
+ public Timeout getSocketTimeout() {
+ return socketTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectionRequestTimeout(Timeout)
+ */
+ public Timeout getConnectionRequestTimeout() {
+ return connectionRequestTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectTimeout(Timeout)
+ */
+ public Timeout getConnectTimeout() {
+ return connectTimeout;
+ }
+
+ @Override
+ protected ConnectionMgmtConfig clone() throws CloneNotSupportedException {
+ return (ConnectionMgmtConfig) super.clone();
+ }
+
+ @Override
+ public String toString() {
+ final StringBuilder builder = new StringBuilder();
+ builder.append("[");
+ builder.append(", proxy=").append(proxy);
+ builder.append(", connectionRequestTimeout=").append(connectionRequestTimeout);
+ builder.append(", connectTimeout=").append(connectTimeout);
+ builder.append("]");
+ return builder.toString();
+ }
+
+ public static ConnectionMgmtConfig.Builder custom() {
+ return new Builder();
+ }
+
+ public static ConnectionMgmtConfig.Builder copy(final ConnectionMgmtConfig config) {
+ return new Builder()
+ .setProxy(config.getProxy())
+ .setConnectionRequestTimeout(config.getConnectionRequestTimeout())
+ .setConnectTimeout(config.getConnectTimeout());
+ }
+
+ public static class Builder {
+
+ private HttpHost proxy;
+ private Timeout socketTimeout;
+ private Timeout connectionRequestTimeout;
+ private Timeout connectTimeout;
+
+ Builder() {
+ super();
+ this.connectionRequestTimeout = DEFAULT_CONNECTION_REQUEST_TIMEOUT;
+ this.connectTimeout = DEFAULT_CONNECT_TIMEOUT;
+ }
+
+
+ /**
+ * @see #setSocketTimeout(Timeout)
+ */
+ public Builder setSocketTimeout(final int soTimeout, final TimeUnit timeUnit) {
+ this.socketTimeout = Timeout.of(soTimeout, timeUnit);
+ return this;
+ }
+
+ /**
+ * Determines the default socket timeout value for I/O operations.
+ * <p>
+ * Default: 3 minutes
Review comment:
This one still holds true for this class.
##########
File path: httpclient5/src/test/java/org/apache/hc/client5/http/examples/ClientConfiguration.java
##########
@@ -172,15 +173,17 @@ public Header parseHeader(final CharArrayBuffer buffer) {
socketFactoryRegistry, PoolConcurrencyPolicy.STRICT, PoolReusePolicy.LIFO, TimeValue.ofMinutes(5),
null, dnsResolver, null);
- // Create socket configuration
- final SocketConfig socketConfig = SocketConfig.custom()
- .setTcpNoDelay(true)
- .build();
// Configure the connection manager to use socket configuration either
// by default or for a specific host.
- connManager.setDefaultSocketConfig(socketConfig);
- // Validate connections after 1 sec of inactivity
- connManager.setValidateAfterInactivity(TimeValue.ofSeconds(10));
+ connManager.setDefaultSocketConfig(SocketConfig.custom()
+ .setTcpNoDelay(true)
+ .build());
+ // Validate connections after 10 sec of inactivity
+ connManager.setDefaultConnectionMgmtConfig(ConnectionMgmtConfig.custom()
+ .setConnectTimeout(Timeout.ofSeconds(30))
+ .setSocketTimeout(Timeout.ofSeconds(30))
Review comment:
When I see the socket config above I am confused that the socket timeout doesn't happen there. General question to other reviewers: Should that be rather read `readTimeout`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601718552
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
I defer to your judgement, I could make a case for either design.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601228940
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -113,10 +116,12 @@
private final HttpClientConnectionOperator connectionOperator;
private final ManagedConnPool<HttpRoute, ManagedHttpClientConnection> pool;
private final HttpConnectionFactory<ManagedHttpClientConnection> connFactory;
+ private final ConcurrentMap<HttpRoute, SocketConfig> socketConfigMap;
+ private final ConcurrentMap<HttpRoute, ConnectionMgmtConfig> connectionMgmtConfigMap;
Review comment:
@carterkozak Using a `Provider` interface for the same end is a great idea. Thank you!
I will re-work the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-808042976
>
>
> @ok2c I have found a few leftovers and a few questions. Please have a look.
Thank you for spotting the issues. Corrected. Please have a look and let me know if you are conceptually OK with the change. We can still move individual settings around in the course of 5.2 development, so this is definitely not the final state of things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-808137217
> Did you push? I still can find a lot of Mgmt in the changeset.
@michael-o I overlooked variable names. Corrected.
Do you want me to move `ConnectionRequestTimeout` to `ConnectionConfig`? I can do that but I suppose at some point we will have to deal with a bunch of angry people demanding those properties to be un-deprecated. `ConnectTimeout` would be a easier to argue about. `ConnectionRequestTimeout` is less so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r598095995
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
Review comment:
@carterkozak @rschmitt @michael-o All right. I will see if I can move `ConnectionMgmtConfig` to connection managers, though I am sure it is a matter of time till someone pops up on the mailing list and demands those settings to be made available on a per request basis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r598096418
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -42,8 +42,6 @@
@Contract(threading = ThreadingBehavior.IMMUTABLE)
public class RequestConfig implements Cloneable {
- private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
@michael-o You are correct. This is a request level parameter. I will keep it in `RequestConfig`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-803380658
>
>
> > Addtionally to the comments: I'd like to understand the followinig:
> > Why has the proxy host has been moved to connection mgmt? In my understanding an HTTP proxy does not operate on socket level, but on protocol level. This would make sense if we'd use SOCKS5. Can you explain?
>
> HTTP proxy do essentially operate on the socket level once a tunnel has been established through it. However, I will deprecate proxy settings in `RequestConfig` in favor of `HttpRoutePlanner`. People should be using `HttpRoutePlanner` for custom routing strategies anyway.
Agreed, sounds better than moving to `ConnMgmtConfig`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-805273488
@rschmitt @carterkozak @rschmitt
Here's the second take:
* new `ConnectionMgmtConfig` structure configurable on the connection manager level
* proxy setting in `RequestConfig` deprecated in favor of `HttpRoutePlanner`
* connect timeout setting in `RequestConfig` deprecated in favor of `ConnectionMgmtConfig`
* `validateAfterInactivity` setting deprecated in favor `ConnectionMgmtConfig`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-807549787
Aside from the minor threading issue, this looks good to me. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602088325
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
I am accepting @ok2c's argumentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] rschmitt commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
rschmitt commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r597929172
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
Review comment:
> Options which apply to the connection pool as a whole
It seems that the current convention is to specify those options directly on the connection manager, e.g. connection TTL, idle connection validation, connection pool queueing and concurrency policies, etc. So if we wanted to make `connectTimeout` specify behavior for the pool as a whole without the ability to overload `connectTimeout` on a per-request basis, I assume we would add that option to `PoolingHttpClientConnectionManagerBuilder` and similar classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601292659
##########
File path: httpclient5-fluent/src/main/java/org/apache/hc/client5/http/fluent/Executor.java
##########
@@ -64,7 +65,9 @@
.useSystemProperties()
.setMaxConnPerRoute(100)
.setMaxConnTotal(200)
- .setValidateAfterInactivity(TimeValue.ofSeconds(10))
+ .setDefaultConnectionMgmtConfig(ConnectionMgmtConfig.custom()
Review comment:
@carterkozak I wonder why we need `Mgmt` at all. Do we have `RequestMmgtConfig`. `ConnectionMgmt..` makes sense when it has influence on the connection tool manager. Then it makes sense, but a full word is better to me too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601855655
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/InternalH2ConnPool.java
##########
@@ -0,0 +1,102 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.async;
+
+import java.net.InetSocketAddress;
+import java.util.concurrent.Future;
+
+import org.apache.hc.client5.http.config.ConnectionConfig;
+import org.apache.hc.core5.concurrent.CallbackContribution;
+import org.apache.hc.core5.concurrent.FutureCallback;
+import org.apache.hc.core5.function.Resolver;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.nio.ssl.TlsStrategy;
+import org.apache.hc.core5.http2.nio.pool.H2ConnPool;
+import org.apache.hc.core5.io.CloseMode;
+import org.apache.hc.core5.io.ModalCloseable;
+import org.apache.hc.core5.reactor.ConnectionInitiator;
+import org.apache.hc.core5.reactor.IOSession;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.hc.core5.util.Timeout;
+
+class InternalH2ConnPool implements ModalCloseable {
+
+ private final H2ConnPool connPool;
+
+ private volatile Resolver<HttpHost, ConnectionConfig> connectionConfigResolver;
+
+ InternalH2ConnPool(final ConnectionInitiator connectionInitiator,
+ final Resolver<HttpHost, InetSocketAddress> addressResolver,
+ final TlsStrategy tlsStrategy) {
+ this.connPool = new H2ConnPool(connectionInitiator, addressResolver, tlsStrategy);
+ }
+
+ public void close(final CloseMode closeMode) {
+ connPool.close(closeMode);
+ }
+
+ public void close() {
+ connPool.close();
+ }
+
+ private ConnectionConfig resolveConnectionConfig(final HttpHost httpHost) {
+ final ConnectionConfig connectionConfig = connectionConfigResolver != null
+ ? connectionConfigResolver.resolve(httpHost) : null;
Review comment:
Potential race which results in an NPE
```suggestion
final Resolver<HttpHost, ConnectionConfig> snapshot = connectionConfigResolver;
final ConnectionConfig connectionConfig = snapshot != null ? snapshot.resolve(httpHost) : null;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r598125042
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -42,8 +42,6 @@
@Contract(threading = ThreadingBehavior.IMMUTABLE)
public class RequestConfig implements Cloneable {
- private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
I actually meant `DEFAULT_CONN_KEEP_ALIVE`. Which did you mean?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602114309
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
Review comment:
Let's leave it as-is, but I still think that a connection lease timeout should be on a pool. See here: https://github.com/apache/httpcomponents-client/pull/296#discussion_r597967488
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r597780313
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
Review comment:
My understanding is that there are two sets of configuration options:
1. Options which apply to the connection pool as a whole
2. Options that may differ based on individual requests
I think the `connectTimeout` should generally apply to an entire pool of connections (within a route anyhow), while the `connectionRequestTimeout` is more likely to be configured on a per-request basis. Although if pooling is disabled, that may not be the case, but in the modern era of http/1.1 and greater it's probably reasonable to assume pooling is used.
It's not clear to me that all of these options should be configurable per-request, while others should. So I suppose my questions are:
Should this be split into two separate configurations for per-pool/route configuration, and per-request configuration?
Should the division between `ConnectionMgmtConfig` and `RequestConfig` be different?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602084880
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java
##########
@@ -221,6 +229,7 @@ private InternalConnectionEndpoint cast(final AsyncConnectionEndpoint endpoint)
LOG.debug("{} endpoint lease request ({}) {}", id, requestTimeout, ConnPoolSupport.formatStats(route, state, pool));
}
final ComplexFuture<AsyncConnectionEndpoint> resultFuture = new ComplexFuture<>(callback);
+ final ConnectionConfig connMgmtConfig = resolveConnectionConfig(route);
Review comment:
Mgmt leftover?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java
##########
@@ -242,7 +251,7 @@ void leaseCompleted(final PoolEntry<HttpRoute, ManagedAsyncClientConnection> poo
@Override
public void completed(final PoolEntry<HttpRoute, ManagedAsyncClientConnection> poolEntry) {
final ManagedAsyncClientConnection connection = poolEntry.getConnection();
- final TimeValue timeValue = PoolingAsyncClientConnectionManager.this.validateAfterInactivity;
+ final TimeValue timeValue = connMgmtConfig != null ? connMgmtConfig.getValidateAfterInactivity() : null;
Review comment:
Mgmt leftover?
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java
##########
@@ -357,6 +365,9 @@ public void release(final AsyncConnectionEndpoint endpoint, final Object state,
host = route.getTargetHost();
}
final InetSocketAddress localAddress = route.getLocalSocketAddress();
+ final ConnectionConfig connMgmtConfig = resolveConnectionConfig(route);
Review comment:
Mgmt leftover?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601451584
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
@michael-o Yes, it would but this decision cannot be made by the connection management layer. It must be made by the HTTP protocol layer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602086388
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
+ }
+
+ /**
+ * @see Builder#setProxy(HttpHost)
+ */
+ public HttpHost getProxy() {
+ return proxy;
+ }
+
+ /**
+ * @see Builder#setSocketTimeout(Timeout)
+ */
+ public Timeout getSocketTimeout() {
+ return socketTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectionRequestTimeout(Timeout)
+ */
+ public Timeout getConnectionRequestTimeout() {
+ return connectionRequestTimeout;
+ }
+
+ /**
+ * @see Builder#setConnectTimeout(Timeout)
+ */
+ public Timeout getConnectTimeout() {
+ return connectTimeout;
+ }
+
+ @Override
+ protected ConnectionMgmtConfig clone() throws CloneNotSupportedException {
+ return (ConnectionMgmtConfig) super.clone();
+ }
+
+ @Override
+ public String toString() {
+ final StringBuilder builder = new StringBuilder();
+ builder.append("[");
+ builder.append(", proxy=").append(proxy);
+ builder.append(", connectionRequestTimeout=").append(connectionRequestTimeout);
+ builder.append(", connectTimeout=").append(connectTimeout);
+ builder.append("]");
+ return builder.toString();
+ }
+
+ public static ConnectionMgmtConfig.Builder custom() {
+ return new Builder();
+ }
+
+ public static ConnectionMgmtConfig.Builder copy(final ConnectionMgmtConfig config) {
+ return new Builder()
+ .setProxy(config.getProxy())
+ .setConnectionRequestTimeout(config.getConnectionRequestTimeout())
+ .setConnectTimeout(config.getConnectTimeout());
+ }
+
+ public static class Builder {
+
+ private HttpHost proxy;
+ private Timeout socketTimeout;
+ private Timeout connectionRequestTimeout;
+ private Timeout connectTimeout;
+
+ Builder() {
+ super();
+ this.connectionRequestTimeout = DEFAULT_CONNECTION_REQUEST_TIMEOUT;
+ this.connectTimeout = DEFAULT_CONNECT_TIMEOUT;
+ }
+
+
+ /**
+ * @see #setSocketTimeout(Timeout)
+ */
+ public Builder setSocketTimeout(final int soTimeout, final TimeUnit timeUnit) {
+ this.socketTimeout = Timeout.of(soTimeout, timeUnit);
+ return this;
+ }
+
+ /**
+ * Determines the default socket timeout value for I/O operations.
+ * <p>
+ * Default: 3 minutes
Review comment:
Still wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601336300
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
>
>
> Another stupid question: `DEFAULT_CONN_KEEP_ALIVE` applies to the lifetime of the connection. Why is is retained here and not moved to the new config class?
The `keep-alive` period is determined on a per request basis, generally based on `Keep-Alive` header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r600896225
##########
File path: httpclient5-fluent/src/main/java/org/apache/hc/client5/http/fluent/Executor.java
##########
@@ -64,7 +65,9 @@
.useSystemProperties()
.setMaxConnPerRoute(100)
.setMaxConnTotal(200)
- .setValidateAfterInactivity(TimeValue.ofSeconds(10))
+ .setDefaultConnectionMgmtConfig(ConnectionMgmtConfig.custom()
Review comment:
I would slightly prefer expanding "Mgmt" to "Management" in method and class names for readability, but I don't have a strong opinion if you feel otherwise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-806973666
ConnectionConfig sounds good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601388031
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
But this will affect the pool object after the request has been completed, doesn't it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602085149
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java
##########
@@ -369,6 +380,10 @@ public void completed(final ManagedAsyncClientConnection connection) {
if (LOG.isDebugEnabled()) {
LOG.debug("{} connected {}", ConnPoolSupport.getId(endpoint), ConnPoolSupport.getId(connection));
}
+ final Timeout socketTimeout = connMgmtConfig.getSocketTimeout();
Review comment:
Mgmt leftover?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-808050445
>
>
> > @ok2c I have found a few leftovers and a few questions. Please have a look.
>
> Thank you for spotting the issues. Corrected. Please have a look and let me know if you are conceptually OK with the change. We can still move individual settings around in the course of 5.2 development, so this is definitely not the final state of things.
Did you push? I still can find a lot of `Mgmt` in the changeset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r597967488
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/ConnectionMgmtConfig.java
##########
@@ -0,0 +1,232 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.client5.http.config;
+
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Timeout;
+
+/**
+ * Immutable class encapsulating connection initialization and management settings.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+public class ConnectionMgmtConfig implements Cloneable {
+
+ private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
+ private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
+
+ public static final ConnectionMgmtConfig DEFAULT = new Builder().build();
+
+ private final HttpHost proxy;
+ private final Timeout socketTimeout;
+ private final Timeout connectionRequestTimeout;
+ private final Timeout connectTimeout;
+
+ /**
+ * Intended for CDI compatibility
+ */
+ protected ConnectionMgmtConfig() {
+ this(null, null, DEFAULT_CONNECTION_REQUEST_TIMEOUT, DEFAULT_CONNECT_TIMEOUT);
+ }
+
+ ConnectionMgmtConfig(
+ final HttpHost proxy,
+ final Timeout socketTimeout,
+ final Timeout connectionRequestTimeout,
+ final Timeout connectTimeout) {
+ super();
+ this.proxy = proxy;
+ this.socketTimeout = socketTimeout;
+ this.connectionRequestTimeout = connectionRequestTimeout;
+ this.connectTimeout = connectTimeout;
Review comment:
I suppor the config on the pool itself. Using Commons Pool or Tomcat JDBC Pool I specify how long it should block until a get a valid option from. This is an immutable config on the pool and not on the request. This should be completely opaque to the client code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r598125181
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -42,8 +42,6 @@
@Contract(threading = ThreadingBehavior.IMMUTABLE)
public class RequestConfig implements Cloneable {
- private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
> I actually meant `DEFAULT_CONN_KEEP_ALIVE`. Which did you mean?
I was referring to DEFAULT_CONNECTION_REQUEST_TIMEOUT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601857878
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -243,6 +245,16 @@ private InternalConnectionEndpoint cast(final ConnectionEndpoint endpoint) {
throw new IllegalStateException("Unexpected endpoint class: " + endpoint.getClass());
}
+ private ConnectionConfig resolveConnectionConfig(final HttpRoute route) {
+ final ConnectionConfig connectionConfig = connectionConfigResolver != null ? connectionConfigResolver.resolve(route) : null;
+ return connectionConfig != null ? connectionConfig : ConnectionConfig.DEFAULT;
+ }
+
+ private SocketConfig resolveSocketConfig(final HttpRoute route) {
+ final SocketConfig socketConfig = socketConfigResolver != null ? socketConfigResolver.resolve(route) : null;
+ return socketConfig != null ? socketConfig : SocketConfig.DEFAULT;
Review comment:
These two methods should use a single volatile read on the resolver as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r600904043
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -495,13 +513,58 @@ public void setDefaultSocketConfig(final SocketConfig defaultSocketConfig) {
this.defaultSocketConfig = defaultSocketConfig;
}
+ /**
+ * @since 5.2
+ */
+ public void setSocketConfig(final HttpRoute route, final SocketConfig socketConfig) {
+ socketConfigMap.put(route, defaultSocketConfig);
+ }
+
+ /**
+ * @since 5.2
+ */
+ public SocketConfig getSocketConfig(final HttpRoute route) {
+ return socketConfigMap.get(route);
+ }
+
+ /**
+ * @since 5.2
+ */
+ public ConnectionMgmtConfig getDefaultConnectionMgmtConfig() {
+ return defaultConnectionMgmtConfig;
+ }
+
+ /**
+ * @since 5.2
+ */
+ public void setDefaultConnectionMgmtConfig(final ConnectionMgmtConfig defaultConnectionMgmtConfig) {
+ this.defaultConnectionMgmtConfig = defaultConnectionMgmtConfig;
+ }
+
+ /**
+ * @since 5.2
+ */
+ public void setConnectionMgmtConfig(final HttpRoute route, final ConnectionMgmtConfig connectionMgmtConfig) {
+ connectionMgmtConfigMap.put(route, connectionMgmtConfig);
+ }
+
+ /**
+ * @since 5.2
+ */
+ public ConnectionMgmtConfig getConnectionMgmtConfig(final HttpRoute route) {
+ return connectionMgmtConfigMap.get(route);
Review comment:
Should this return the default configuration when no route-specific configuration exists?
`return connectionMgmtConfigMap.getOrDefault(route, defaultConnectionMgmtConfig);`
It's not clear if this is intended to expose the configuration that we use for a given route, or provide access to route-specific overrides.
I think the same argument can be made for the SocketConfig.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r602103397
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
Review comment:
@michael-o This really depends how one looks at it. This setting is used by each and every request and as such makes sense at the request level. However, indeed, the inner machinery is entirely in the connection manager's responsibility.
I initially moved that parameter to `ConnectionConfig` but if I remember it correctly you questioned that decision.
Basically as far as I am concerned it can be either way. Blonds or Brunettes, so to speak. However I tend to see it more of a request level setting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] michael-o commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r598137696
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -42,8 +42,6 @@
@Contract(threading = ThreadingBehavior.IMMUTABLE)
public class RequestConfig implements Cloneable {
- private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
That's also true...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#issuecomment-807505992
@carterkozak @michael-o Please do another pass.
The only big change compared to the previous change-set is the use of config resolvers instead of internal maps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r601615151
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java
##########
@@ -43,7 +43,6 @@
public class RequestConfig implements Cloneable {
private static final Timeout DEFAULT_CONNECTION_REQUEST_TIMEOUT = Timeout.ofMinutes(3);
- private static final Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3);
private static final TimeValue DEFAULT_CONN_KEEP_ALIVE = TimeValue.ofMinutes(3);
Review comment:
Based on the request and response I think there are three results:
1. Do not keep the connection alive (`Connection: close` response header)
2. keep the connection alive for a specific amount of time `Keep-Alive: timeout=30`
3. keep the connection alive for an indeterminate amount of time so the default value can be used (http/1.1 without specifying close)
If memory serves, we use `null` for [1] (connection is not kept alive) and return a value based on either a keep-alive timeout value [2] or the default [3] when configured. If we can update the abstraction to differentiate states [1] from [3] I think the default configuration makes sense in the connection management configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] carterkozak commented on a change in pull request #296: Connection management configuration
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #296:
URL: https://github.com/apache/httpcomponents-client/pull/296#discussion_r600907813
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java
##########
@@ -113,10 +116,12 @@
private final HttpClientConnectionOperator connectionOperator;
private final ManagedConnPool<HttpRoute, ManagedHttpClientConnection> pool;
private final HttpConnectionFactory<ManagedHttpClientConnection> connFactory;
+ private final ConcurrentMap<HttpRoute, SocketConfig> socketConfigMap;
+ private final ConcurrentMap<HttpRoute, ConnectionMgmtConfig> connectionMgmtConfigMap;
Review comment:
Should we worry about routes building up over time without a path to removal and garbage collection?
I wonder if we should provide an interface to optionally build a route-specific socket and ConnMgmtConfig the first time a route is used, which can be forgotten and recalculated later if all connections to that route are cleared (or even naively recalculated for each connection, allowing implementations to cache internally).
```java
interface RouteBasedSocketConfigProvider {
SocketConfig socketConfigForRoute(HttpRoute route);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org