This is an automated email from the ASF dual-hosted git repository.
ffang pushed a commit to branch 3.6.x-fixes
in repository https://gitbox.apache.org/repos/asf/cxf.git
The following commit(s) were added to refs/heads/3.6.x-fixes by this push:
new 5164861b872 CXF-9266: Keep the settings of a shared HttpClient from
other conduits (#3673)
5164861b872 is described below
commit 5164861b8729ab3485dfc0f48c01dfc6ddb833e2
Author: Freeman(Yue) Fang <[email protected]>
AuthorDate: Mon Nov 9 18:49:02 2026 -0500
CXF-9266: Keep the settings of a shared HttpClient from other conduits
(#3673)
* CXF-9266: Keep the settings of a shared HttpClient from other conduits
HttpClientHTTPConduit shares one HttpClient between conduits with equal
settings. The cache kept the creating conduit's own HTTPClientPolicy and
TLSClientParameters as the key of the shared client, and the client's
ProxySelector read that conduit's policy on every request. So when a
conduit's settings changed after its first call, the conduits sharing its
client were routed through its new proxy, a conduit's own new proxy was
ignored, and a conduit created later with the new settings got the client
built from the old ones, including its SSLContext and connection timeout.
Keep copies of the settings a client is built from, as its key and for its
ProxySelector, and move a conduit to a matching client when its settings no
longer match the ones its client was built from, as is already done when the
HTTPS address changes. The version and the non proxy hosts are compared too,
as the client is built from them. A released client is removed from the
cache by identity.
* CXF-9266: Match a shared HttpClient only on the settings it is built from
The client of a conduit was replaced whenever any setting of its policy
changed, including settings applied to each request such as the receive
timeout, and a conduit switching back and forth between two settings got
a new client each time, while the clients it stopped using were only
released on close.
Compare only the settings the client is built from: the connection
timeout, the proxy, the non proxy hosts and the version, and the TLS
parameters. Copy exactly these settings and deep-copy the cipher suites
filter, without falling back to the live objects. Take back a client
the conduit stopped using when it switches back to its settings.
TLSClientParameters.equals and hashCode now include the hostname verifier,
which the client's trust managers are built with.
* CXF-9266: Key a shared HttpClient on the effective connection timeout and
version
- The client is built with the connection timeout and HTTP version in effect
for the request, which the message can override (CONNECTION_TIMEOUT,
FORCE_HTTP_VERSION). Use those, not the policy's values, as its key and in
the reset check, so that a conduit neither shares a client built with
another conduit's request context settings, nor ignores its own.
- Clients dropped because the HTTPS address changed, and the ones deferred
before, are no longer taken back when switching back to their settings;
they are only released on close().
- close() takes the current and deferred clients under the initialization
lock, so a concurrent request cannot take a deferred client back after it.
- The certificate constraints are no longer part of the key. The client is
not built from them, they are checked for each response, so changing them,
in place or by replacing them, keeps the client.
* CXF-9266: Do not use a shared HttpClient built for http for https
A client built for an http address is built without the TLS parameters, so
it uses the default SSLContext, yet it was keyed on them. A conduit for
https
with the same TLS parameters, or the conduit itself when its address changed
to https, then used it, without its own trust managers, key managers, cipher
suites or protocols. Key such a client without TLS parameters instead.
* CXF-9266: Copy the TLS client parameters with TLSClientParameters.copy()
The copy of the TLS client parameters the conduit keeps for its HttpClient
is now made by TLSClientParameters.copy(), which copies the arrays, the
lists and the cipher suites filter, and shares the objects they hold. The
conduit still leaves the certificate constraints out of its copy, as the
client is not built from them.
* CXF-9266: Do not take back a client the conduit has stopped using
Taking back a client when the conduit switches back to its settings made
the release of the clients hard to follow, for the narrow case of settings
that change back and forth. A conduit now always gets its client from the
cache, and keeps a single reference to a shared client it gets back, so
that switching back and forth does not grow the clients released on
close().
* CXF-9266: Keep the deferred clients in a plain list
The deferred clients are only accessed under the initialization lock, so
they do not need a concurrent queue.
* CXF-9266: Check that taking back a deferred client does not close it
The conduit holds the deferred reference and the one it has just acquired
from the cache, so releasing the deferred one should never close the
client. Fail clearly if it does.
(cherry picked from commit 0f0f94432cbb05170dbdc99eb90c74ccfb9ed5a1)
Backport: the named groups are left out, as they are not part of 4.1.x.
---
.../configuration/jsse/TLSClientParameters.java | 39 ++
.../jsse/TLSClientParametersTest.java | 75 ++++
.../cxf/transport/http/HttpClientHTTPConduit.java | 200 +++++++---
.../HttpClientHTTPConduitSharedClientTest.java | 409 +++++++++++++++++++++
4 files changed, 664 insertions(+), 59 deletions(-)
diff --git
a/core/src/main/java/org/apache/cxf/configuration/jsse/TLSClientParameters.java
b/core/src/main/java/org/apache/cxf/configuration/jsse/TLSClientParameters.java
index c7cebf06651..cdf89b827f6 100644
---
a/core/src/main/java/org/apache/cxf/configuration/jsse/TLSClientParameters.java
+++
b/core/src/main/java/org/apache/cxf/configuration/jsse/TLSClientParameters.java
@@ -18,6 +18,7 @@
*/
package org.apache.cxf.configuration.jsse;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
@@ -32,6 +33,7 @@ import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSocketFactory;
import org.apache.cxf.common.util.SystemPropertyAction;
+import org.apache.cxf.configuration.security.FiltersType;
/**
* This class extends {@link TLSParameterBase} with client-specific
@@ -189,6 +191,9 @@ public class TLSClientParameters extends TLSParameterBase {
if (sslContext != null) {
hash = hash * 41 + System.identityHashCode(sslContext);
}
+ if (hostnameVerifier != null) {
+ hash = hash * 41 + System.identityHashCode(hostnameVerifier);
+ }
hash = hash(hash, useHttpsURLConnectionDefaultSslSocketFactory);
hash = hash(hash, useHttpsURLConnectionDefaultHostnameVerifier);
hash = hash(hash, sslCacheTimeout);
@@ -240,6 +245,7 @@ public class TLSClientParameters extends TLSParameterBase {
boolean eq = disableCNCheck == that.disableCNCheck;
eq &= sslSocketFactory == that.sslSocketFactory;
eq &= sslContext == that.sslContext;
+ eq &= hostnameVerifier == that.hostnameVerifier;
eq &= useHttpsURLConnectionDefaultSslSocketFactory ==
that.useHttpsURLConnectionDefaultSslSocketFactory;
eq &= useHttpsURLConnectionDefaultHostnameVerifier ==
that.useHttpsURLConnectionDefaultHostnameVerifier;
eq &= sslCacheTimeout == that.sslCacheTimeout;
@@ -300,6 +306,39 @@ public class TLSClientParameters extends TLSParameterBase {
this.sslContext = sslContext;
}
+ /**
+ * Returns a copy of these parameters, which later changes to them do not
affect. The arrays, the
+ * lists and the cipher suites filter are copied, while the objects they
hold, such as the key and
+ * trust managers, are shared, as are the certificate constraints.
+ */
+ public TLSClientParameters copy() {
+ final TLSClientParameters copy = new TLSClientParameters();
+ copy.disableCNCheck = disableCNCheck;
+ copy.sslSocketFactory = sslSocketFactory;
+ copy.sslCacheTimeout = sslCacheTimeout;
+ copy.useHttpsURLConnectionDefaultSslSocketFactory =
useHttpsURLConnectionDefaultSslSocketFactory;
+ copy.useHttpsURLConnectionDefaultHostnameVerifier =
useHttpsURLConnectionDefaultHostnameVerifier;
+ copy.hostnameVerifier = hostnameVerifier;
+ copy.sslContext = sslContext;
+ if (serverNames != null) {
+ copy.serverNames = new ArrayList<>(serverNames);
+ }
+ copy.keyManagers = keyManagers == null ? null : keyManagers.clone();
+ copy.trustManagers = trustManagers == null ? null :
trustManagers.clone();
+ copy.provider = provider;
+ copy.ciphersuites = ciphersuites == null ? null : new
ArrayList<>(ciphersuites);
+ if (cipherSuiteFilters != null) {
+ copy.cipherSuiteFilters = new FiltersType();
+
copy.cipherSuiteFilters.getInclude().addAll(cipherSuiteFilters.getInclude());
+
copy.cipherSuiteFilters.getExclude().addAll(cipherSuiteFilters.getExclude());
+ }
+ copy.certConstraints = certConstraints;
+ copy.secureRandom = secureRandom;
+ copy.protocol = protocol;
+ copy.certAlias = certAlias;
+ return copy;
+ }
+
public static String[] getPreferredClientProtocols() {
if (!shouldIgnoreConfiguredCLientProtocols()) {
final Collection<String> configured =
getConfiguredCLientProtocols();
diff --git
a/core/src/test/java/org/apache/cxf/configuration/jsse/TLSClientParametersTest.java
b/core/src/test/java/org/apache/cxf/configuration/jsse/TLSClientParametersTest.java
index f7090e1660c..b694f0a7d90 100644
---
a/core/src/test/java/org/apache/cxf/configuration/jsse/TLSClientParametersTest.java
+++
b/core/src/test/java/org/apache/cxf/configuration/jsse/TLSClientParametersTest.java
@@ -19,12 +19,28 @@
package org.apache.cxf.configuration.jsse;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Arrays;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+
+import org.apache.cxf.configuration.security.CertificateConstraintsType;
+import org.apache.cxf.configuration.security.FiltersType;
+
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
public class TLSClientParametersTest {
private String oldProtocols;
@@ -74,4 +90,63 @@ public class TLSClientParametersTest {
"TLSv1.2",
"TLSv1.3"));
}
+
+ @Test
+ public void testCopy() throws Exception {
+ TrustManagerFactory tmf =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ tmf.init((java.security.KeyStore)null);
+ TrustManager[] trustManagers = tmf.getTrustManagers();
+ TLSClientParameters params = new TLSClientParameters();
+ params.setDisableCNCheck(true);
+ params.setSslCacheTimeout(100);
+ params.setUseHttpsURLConnectionDefaultSslSocketFactory(true);
+ params.setUseHttpsURLConnectionDefaultHostnameVerifier(true);
+ params.setHostnameVerifier((hostname, session) -> true);
+ params.setServerNames(new ArrayList<>(Arrays.asList("server")));
+ params.setKeyManagers(new KeyManager[0]);
+ params.setTrustManagers(trustManagers.clone());
+ params.setJsseProvider("SunJSSE");
+ params.setCipherSuites(new
ArrayList<>(Arrays.asList("TLS_AES_128_GCM_SHA256")));
+ FiltersType filters = new FiltersType();
+ filters.getInclude().add(".*_AES_.*");
+ filters.getExclude().add(".*_NULL_.*");
+ params.setCipherSuitesFilter(filters);
+ params.setCertConstraints(new CertificateConstraintsType());
+ params.setSecureRandom(new SecureRandom());
+ params.setSecureSocketProtocol("TLSv1.3");
+ params.setCertAlias("alias");
+
+ TLSClientParameters copy = params.copy();
+ assertEquals(params, copy);
+ assertEquals(params.hashCode(), copy.hashCode());
+ assertEquals(100, copy.getSslCacheTimeout());
+ assertSame(params.getHostnameVerifier(), copy.getHostnameVerifier());
+ assertArrayEquals(params.getTrustManagers(), copy.getTrustManagers());
+ assertSame(params.getCertConstraints(), copy.getCertConstraints());
+ assertSame(params.getSecureRandom(), copy.getSecureRandom());
+ assertEquals("SunJSSE", copy.getJsseProvider());
+ assertEquals("alias", copy.getCertAlias());
+
+ // Later changes to the parameters, made in place, do not affect the
copy
+ params.getServerNames().add("other");
+ params.getTrustManagers()[0] = null;
+ params.getCipherSuites().add("TLS_AES_256_GCM_SHA384");
+ params.getCipherSuitesFilter().getInclude().add(".*");
+ assertEquals(Arrays.asList("server"), copy.getServerNames());
+ assertArrayEquals(trustManagers, copy.getTrustManagers());
+ assertEquals(Arrays.asList("TLS_AES_128_GCM_SHA256"),
copy.getCipherSuites());
+ assertEquals(Arrays.asList(".*_AES_.*"),
copy.getCipherSuitesFilter().getInclude());
+ assertEquals(Arrays.asList(".*_NULL_.*"),
copy.getCipherSuitesFilter().getExclude());
+ assertNotEquals(params, copy);
+ }
+
+ @Test
+ public void testCopyOfDefaults() {
+ TLSClientParameters params = new TLSClientParameters();
+ TLSClientParameters copy = params.copy();
+ assertEquals(params, copy);
+ assertNull(copy.getServerNames());
+ assertNull(copy.getTrustManagers());
+ assertNull(copy.getCipherSuitesFilter());
+ }
}
diff --git
a/rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java
b/rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java
index c2cecc9df0a..472f83dd9c6 100644
---
a/rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java
+++
b/rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java
@@ -61,16 +61,14 @@ import java.security.cert.Certificate;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Arrays;
-import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Optional;
-import java.util.Queue;
import java.util.Set;
import java.util.TreeSet;
import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Flow;
import java.util.concurrent.Flow.Subscriber;
@@ -78,6 +76,7 @@ import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Consumer;
import java.util.function.Supplier;
import javax.net.ssl.SSLContext;
@@ -94,7 +93,6 @@ import org.apache.cxf.io.CacheAndWriteOutputStream;
import org.apache.cxf.message.Message;
import org.apache.cxf.message.MessageUtils;
import org.apache.cxf.service.model.EndpointInfo;
-import org.apache.cxf.transport.http.policy.impl.ClientPolicyCalculator;
import org.apache.cxf.transport.https.HttpsURLConnectionInfo;
import org.apache.cxf.transport.https.SSLUtils;
import org.apache.cxf.transports.http.configuration.HTTPClientPolicy;
@@ -111,16 +109,20 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
volatile RefCount<HttpClient> clientRef;
volatile URI sslURL;
private final ReentrantLock initializationLock = new ReentrantLock();
- private final Queue<RefCount<HttpClient>> deferredClientRefs = new
ConcurrentLinkedQueue<>();
+ // clients this conduit has stopped using, released on close() as they
could still be in use,
+ // only accessed under initializationLock
+ private final List<RefCount<HttpClient>> deferredClientRefs = new
ArrayList<>();
private static final class RefCount<T extends HttpClient> {
private final AtomicLong count;
+ // Copies of the settings the client was built from, no TLS parameters
if it was built for http
private final TLSClientParameters clientParameters;
private final HTTPClientPolicy policy;
private final T client;
- private final Runnable finalizer;
+ private final Consumer<RefCount<T>> finalizer;
- RefCount(T client, HTTPClientPolicy policy, TLSClientParameters
clientParameters, Runnable finalizer) {
+ RefCount(T client, HTTPClientPolicy policy, TLSClientParameters
clientParameters,
+ Consumer<RefCount<T>> finalizer) {
this.client = client;
this.policy = policy;
this.clientParameters = clientParameters;
@@ -142,7 +144,7 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
void release() {
if (count.decrementAndGet() == 0) {
- finalizer.run();
+ finalizer.accept(this);
if (client instanceof AutoCloseable) {
try {
@@ -186,12 +188,11 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
return client;
}
- HTTPClientPolicy policy() {
- return policy;
- }
-
- public TLSClientParameters clientParameters() {
- return clientParameters;
+ /**
+ * Whether the client was built from settings equal to these ones
+ */
+ boolean matches(HTTPClientPolicy otherPolicy, TLSClientParameters
otherClientParameters) {
+ return samePolicy(policy, otherPolicy) &&
Objects.equals(clientParameters, otherClientParameters);
}
}
@@ -199,15 +200,18 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
private static final int MAX_SIZE = 100; // Keeping at most 100 clients
private final List<RefCount<HttpClient>> clients = new ArrayList<>();
- private final ClientPolicyCalculator cpc = new
ClientPolicyCalculator();
private final ReentrantLock lock = new ReentrantLock();
+ /**
+ * The policy and the client parameters must be copies that nothing
else changes, as they
+ * stay the key of the cached client
+ */
RefCount<HttpClient> computeIfAbsent(final boolean shareHttpClient,
final HTTPClientPolicy policy,
final TLSClientParameters clientParameters, final
Supplier<HttpClient> supplier) {
// Do not share if it is not allowed for the conduit or cache
capacity is exceeded
if (!shareHttpClient || clients.size() >= MAX_SIZE) {
- return new RefCount<HttpClient>(supplier.get(), policy,
clientParameters, () -> { });
+ return new RefCount<HttpClient>(supplier.get(), policy,
clientParameters, r -> { });
}
lock.lock();
@@ -217,7 +221,7 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
if (p.isClosed()) { // skip closed but not yet removed
clients
continue;
}
- if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
+ if (p.matches(policy, clientParameters)) {
return p.acquire();
}
} catch (final IllegalStateException ex) {
@@ -228,7 +232,7 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
final HttpClient client = supplier.get();
final RefCount<HttpClient> clientRef = new
RefCount<HttpClient>(client, policy, clientParameters,
- () -> this.remove(policy, clientParameters));
+ this::remove);
clients.add(clientRef);
return clientRef;
@@ -237,17 +241,10 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
}
}
- void remove(final HTTPClientPolicy policy, final TLSClientParameters
clientParameters) {
+ void remove(final RefCount<HttpClient> clientRef) {
lock.lock();
try {
- final Iterator<RefCount<HttpClient>> iterator =
clients.iterator();
- while (iterator.hasNext()) {
- final RefCount<HttpClient> p = iterator.next();
- if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
- iterator.remove();
- break;
- }
- }
+ clients.remove(clientRef);
} finally {
lock.unlock();
}
@@ -258,6 +255,90 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
super(b, ei, t);
}
+ /**
+ * Compares only the settings a client is built from: the connection
timeout, the proxy and the
+ * version. The other settings are applied to each request, so changing
them keeps the client
+ */
+ private static boolean samePolicy(HTTPClientPolicy p1, HTTPClientPolicy
p2) {
+ return p1.getConnectionTimeout() == p2.getConnectionTimeout()
+ && Objects.equals(p1.getProxyServer(), p2.getProxyServer())
+ && Objects.equals(p1.getProxyServerPort(), p2.getProxyServerPort())
+ && p1.getProxyServerType() == p2.getProxyServerType()
+ && Objects.equals(p1.getNonProxyHosts(), p2.getNonProxyHosts())
+ && Objects.equals(p1.getVersion(), p2.getVersion());
+ }
+
+ /**
+ * Copies the settings a client is built from, so that changes to the
conduit's policy do not change
+ * the key of the cached client, or the proxies it uses. The connection
timeout and the version are
+ * the ones in effect for the request, which the message can override
+ */
+ private static HTTPClientPolicy copyOf(HTTPClientPolicy policy, int
connectionTimeout, String version) {
+ final HTTPClientPolicy copy = new HTTPClientPolicy();
+ copy.setConnectionTimeout(connectionTimeout);
+ if (policy.isSetProxyServer()) {
+ copy.setProxyServer(policy.getProxyServer());
+ }
+ if (policy.isSetProxyServerPort()) {
+ copy.setProxyServerPort(policy.getProxyServerPort());
+ }
+ if (policy.isSetProxyServerType()) {
+ copy.setProxyServerType(policy.getProxyServerType());
+ }
+ if (policy.isSetNonProxyHosts()) {
+ copy.setNonProxyHosts(policy.getNonProxyHosts());
+ }
+ if (version != null) {
+ copy.setVersion(version);
+ }
+ return copy;
+ }
+
+ private static TLSClientParameters copyOf(TLSClientParameters params) {
+ final TLSClientParameters copy = params.copy();
+ // the certificate constraints are left out: the client is not built
from them, they are
+ // checked for each response, so changing them must not replace the
client
+ copy.setCertConstraints(null);
+ return copy;
+ }
+
+ /**
+ * Stops using the client, without releasing it immediately since it could
be in use,
+ * by moving it off to the deferred release list
+ */
+ private void resetClient(RefCount<HttpClient> ref) {
+ initializationLock.lock();
+ try {
+ // only once, if several requests reset the same client at the
same time
+ if (ref != null && clientRef == ref) {
+ deferredClientRefs.add(ref);
+ clientRef = null;
+ }
+ } finally {
+ initializationLock.unlock();
+ }
+ }
+
+ /**
+ * Stops using the client if the HTTPS address or the settings it was
built from have changed
+ */
+ private void resetClientIfChanged(Message message, URI uri,
HTTPClientPolicy clientPolicy,
+ TLSClientParameters clientParameters) {
+ if (sslURL != null && isSslTargetDifferent(sslURL, uri)) {
+ sslURL = null;
+
+ // Reset the client in case of HTTPS URL change
+ if (MessageUtils.getContextualBoolean(message,
HTTPS_RESET_HTTPCLIENT_CONDUIT, true)) {
+ resetClient(clientRef);
+ }
+ }
+ // Reset the client if the settings it was built from have changed
since
+ final RefCount<HttpClient> ref = clientRef;
+ if (ref != null && !ref.matches(clientPolicy, clientParameters)) {
+ resetClient(ref);
+ }
+ }
+
private static Set<String> getRestrictedHeaders() {
Set<String> headers = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
headers.addAll(Set.of("Connection", "Content-Length", "Expect",
"Host", "Upgrade"));
@@ -289,22 +370,21 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
* Close the conduit
*/
public void close() {
- // release each reference only once, even if the conduit is closed
concurrently
- final RefCount<HttpClient> ref;
+ // release each reference only once, even if the conduit is closed
concurrently, and take
+ // them all under the lock, so that a concurrent request does not take
a deferred one back
+ final List<RefCount<HttpClient>> refs = new ArrayList<>();
initializationLock.lock();
try {
- ref = clientRef;
+ if (clientRef != null) {
+ refs.add(clientRef);
+ }
clientRef = null;
+ refs.addAll(deferredClientRefs);
+ deferredClientRefs.clear();
} finally {
initializationLock.unlock();
}
- if (ref != null) {
- ref.release();
- }
- RefCount<HttpClient> deferred;
- while ((deferred = deferredClientRefs.poll()) != null) {
- deferred.release();
- }
+ refs.forEach(RefCount::release);
defaultAddress = null;
super.close();
}
@@ -384,22 +464,18 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
return;
}
- if (sslURL != null && isSslTargetDifferent(sslURL, uri)) {
- sslURL = null;
-
- // Reset the client in case of HTTPS URL change
- final boolean httpsResetHttpClient =
MessageUtils.getContextualBoolean(message,
- HTTPS_RESET_HTTPCLIENT_CONDUIT, true);
- if (httpsResetHttpClient) {
- final RefCount<HttpClient> ref = clientRef;
- // Do not release client immediately since it could be in use,
instead
- // move it off to deferred release queue.
- if (ref != null) {
- deferredClientRefs.add(ref);
- clientRef = null;
- }
- }
+ final int ctimeout = determineConnectionTimeout(message, csPolicy);
+ String verc =
(String)message.getContextualProperty(FORCE_HTTP_VERSION);
+ if (verc == null) {
+ verc = csPolicy.getVersion();
}
+ // the client keeps copies of the settings it is built from, so that
it is not
+ // affected by later changes to the settings of this, or any other,
conduit
+ final HTTPClientPolicy clientPolicy = copyOf(csPolicy, ctimeout, verc);
+ // a client for http is built without the TLS parameters, so it must
not be used for https
+ final TLSClientParameters clientParametersCopy =
+ "https".equals(uri.getScheme()) ? copyOf(clientParameters) : null;
+ resetClientIfChanged(message, uri, clientPolicy, clientParametersCopy);
// If the HTTP_REQUEST_METHOD is not set, the default is "POST".
String httpRequestMethod =
(String)message.get(Message.HTTP_REQUEST_METHOD);
@@ -410,8 +486,7 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
RefCount<HttpClient> cl = clientRef;
if (cl == null) {
- int ctimeout = determineConnectionTimeout(message, csPolicy);
- ProxySelector ps = new ProxyFactoryProxySelector(proxyFactory,
csPolicy);
+ ProxySelector ps = new ProxyFactoryProxySelector(proxyFactory,
clientPolicy);
HttpClient.Builder cb = HttpClient.newBuilder()
.proxy(ps)
@@ -454,10 +529,6 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
throw new IOException(e);
}
}
- String verc =
(String)message.getContextualProperty(FORCE_HTTP_VERSION);
- if (verc == null) {
- verc = csPolicy.getVersion();
- }
if ("1.1".equals(HTTP_VERSION) || "1.1".equals(verc)) {
cb.version(Version.HTTP_1_1);
}
@@ -469,7 +540,18 @@ public class HttpClientHTTPConduit extends
URLConnectionHTTPConduit {
if (cl == null) {
final boolean shareHttpClient =
MessageUtils.getContextualBoolean(message,
SHARE_HTTPCLIENT_CONDUIT, true);
- cl = CLIENTS_CACHE.computeIfAbsent(shareHttpClient,
csPolicy, clientParameters, () -> cb.build());
+ cl = CLIENTS_CACHE.computeIfAbsent(shareHttpClient,
clientPolicy, clientParametersCopy,
+ () -> cb.build());
+ // the cache may hand back a shared client this conduit
has stopped using, when it
+ // switches back to its settings or its HTTPS address
changes. Keep a single reference
+ if (deferredClientRefs.remove(cl)) {
+ cl.release();
+ // the conduit held the deferred reference and has
just acquired another one,
+ // so the release should never close the client
+ if (cl.isClosed()) {
+ throw new IllegalStateException("The client has
been closed already");
+ }
+ }
if (!"https".equals(uri.getScheme())
&&
!KNOWN_HTTP_VERBS_WITH_NO_CONTENT.contains(httpRequestMethod)
diff --git
a/rt/transports/http/src/test/java/org/apache/cxf/transport/http/HttpClientHTTPConduitSharedClientTest.java
b/rt/transports/http/src/test/java/org/apache/cxf/transport/http/HttpClientHTTPConduitSharedClientTest.java
new file mode 100644
index 00000000000..80e45e7863f
--- /dev/null
+++
b/rt/transports/http/src/test/java/org/apache/cxf/transport/http/HttpClientHTTPConduitSharedClientTest.java
@@ -0,0 +1,409 @@
+/**
+ * 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.
+ */
+
+package org.apache.cxf.transport.http;
+
+import java.lang.reflect.Field;
+import java.net.InetSocketAddress;
+import java.net.Proxy;
+import java.net.URI;
+import java.net.http.HttpClient;
+import java.security.cert.X509Certificate;
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Optional;
+import java.util.TreeSet;
+
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.X509TrustManager;
+
+import org.apache.cxf.Bus;
+import org.apache.cxf.bus.extension.ExtensionManagerBus;
+import org.apache.cxf.configuration.jsse.TLSClientParameters;
+import org.apache.cxf.configuration.security.CertificateConstraintsType;
+import org.apache.cxf.configuration.security.DNConstraintsType;
+import org.apache.cxf.configuration.security.FiltersType;
+import org.apache.cxf.message.Message;
+import org.apache.cxf.message.MessageImpl;
+import org.apache.cxf.phase.PhaseInterceptorChain;
+import org.apache.cxf.service.model.EndpointInfo;
+import org.apache.cxf.transports.http.configuration.HTTPClientPolicy;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertSame;
+
+/**
+ * Conduits with equal settings share one HttpClient. Changing the settings of
a conduit after its
+ * first call must neither change the client of the other conduits, nor be
ignored for the conduit itself.
+ */
+public class HttpClientHTTPConduitSharedClientTest {
+ private static final URI TARGET =
URI.create("http://backend.example/test";);
+
+ private Bus bus;
+ private final List<HttpClientHTTPConduit> conduits = new ArrayList<>();
+
+ @Before
+ public void setUp() {
+ bus = new ExtensionManagerBus();
+ }
+
+ @After
+ public void tearDown() {
+ conduits.forEach(HttpClientHTTPConduit::close);
+ bus.shutdown(true);
+ }
+
+ @Test
+ public void testConduitsWithEqualSettingsShareClient() throws Exception {
+ HttpClient client1 = prepare(newConduit("http"));
+ HttpClient client2 = prepare(newConduit("http"));
+ assertSame(client1, client2);
+ }
+
+ @Test
+ public void testProxyOfAnotherConduitIsNotUsed() throws Exception {
+ HttpClientHTTPConduit conduit1 = newConduit("http");
+ HttpClientHTTPConduit conduit2 = newConduit("http");
+ assertSame(prepare(conduit1), prepare(conduit2));
+
+ // only conduit 1 is configured with a proxy
+ setProxy(conduit1.getClient(), "proxy1.example", 8080);
+ assertEquals("proxy1.example", proxyHost(prepare(conduit1)));
+ assertEquals(Proxy.Type.DIRECT, proxy(prepare(conduit2)).type());
+ }
+
+ @Test
+ public void testOwnProxyIsUsed() throws Exception {
+ HttpClientHTTPConduit conduit1 = newConduit("http");
+ HttpClientHTTPConduit conduit2 = newConduit("http");
+ assertSame(prepare(conduit1), prepare(conduit2));
+
+ // only conduit 2 is configured with a proxy
+ setProxy(conduit2.getClient(), "proxy2.example", 3128);
+ assertEquals("proxy2.example", proxyHost(prepare(conduit2)));
+ assertEquals(Proxy.Type.DIRECT, proxy(prepare(conduit1)).type());
+ }
+
+ @Test
+ public void testChangedConnectionTimeout() throws Exception {
+ HttpClientHTTPConduit conduit1 = newConduit("http");
+ HttpClient client1 = prepare(conduit1);
+ assertEquals(Optional.of(Duration.ofSeconds(30)),
client1.connectTimeout());
+
+ // conduit 1 changes its connection timeout after its first call
+ conduit1.getClient().setConnectionTimeout(1000);
+ assertEquals(Optional.of(Duration.ofSeconds(1)),
prepare(conduit1).connectTimeout());
+
+ // a conduit configured like conduit 1 at first still gets a client
with the old timeout
+ assertSame(client1, prepare(newConduit("http")));
+
+ // and a conduit configured with the new timeout from the start gets
the new one
+ HttpClientHTTPConduit conduit3 = newConduit("http");
+ conduit3.getClient().setConnectionTimeout(1000);
+ assertEquals(Optional.of(Duration.ofSeconds(1)),
prepare(conduit3).connectTimeout());
+ }
+
+ @Test
+ public void testChangedTrustManagersAreNotShared() throws Exception {
+ TrustManager trustAll = new TestTrustManager();
+ TrustManager[] strict = {new TestTrustManager()};
+
+ HttpClientHTTPConduit conduit1 = newConduit("https");
+ TLSClientParameters tls1 = new TLSClientParameters();
+ tls1.setTrustManagers(new TrustManager[] {trustAll});
+ conduit1.setTlsClientParameters(tls1);
+ HttpClient trustAllClient = prepare(conduit1);
+
+ // conduit 1 is switched to the strict trust managers after its first
call
+ tls1.setTrustManagers(strict);
+ HttpClient strictClient = prepare(conduit1);
+ assertNotSame(trustAllClient, strictClient);
+
+ // a conduit configured with the strict trust managers from the start
+ // must not get the client built with the trust-all trust manager
+ HttpClientHTTPConduit conduit2 = newConduit("https");
+ TLSClientParameters tls2 = new TLSClientParameters();
+ tls2.setTrustManagers(strict);
+ conduit2.setTlsClientParameters(tls2);
+ HttpClient client2 = prepare(conduit2);
+ assertNotSame(trustAllClient, client2);
+ assertSame(strictClient, client2);
+ }
+
+ @Test
+ public void testHostnameVerifierIsNotShared() throws Exception {
+ TrustManager[] trustManagers = {new TestTrustManager()};
+ HttpClientHTTPConduit conduit1 = newConduit("https");
+ TLSClientParameters tls1 = new TLSClientParameters();
+ tls1.setTrustManagers(trustManagers);
+ tls1.setHostnameVerifier((host, session) -> true);
+ conduit1.setTlsClientParameters(tls1);
+
+ // the hostname verifier is part of the client's trust managers
+ HttpClientHTTPConduit conduit2 = newConduit("https");
+ TLSClientParameters tls2 = new TLSClientParameters();
+ tls2.setTrustManagers(trustManagers);
+ conduit2.setTlsClientParameters(tls2);
+ assertNotSame(prepare(conduit1), prepare(conduit2));
+ }
+
+ @Test
+ public void testChangedCipherSuitesFilter() throws Exception {
+ HttpClientHTTPConduit conduit = newConduit("https");
+ TLSClientParameters tls = new TLSClientParameters();
+ FiltersType filters = new FiltersType();
+ filters.getInclude().add(".*_AES_128_.*");
+ tls.setCipherSuitesFilter(filters);
+ conduit.setTlsClientParameters(tls);
+ HttpClient client = prepare(conduit);
+
+ // the filter of the conduit is changed in place after its first call
+ filters.getInclude().add(".*_AES_256_.*");
+ assertNotSame(client, prepare(conduit));
+ }
+
+ @Test
+ public void testChangedCertConstraintsKeepClient() throws Exception {
+ HttpClientHTTPConduit conduit = newConduit("https");
+ TLSClientParameters tls = new TLSClientParameters();
+ tls.setCertConstraints(certConstraints("CN=one"));
+ conduit.setTlsClientParameters(tls);
+ HttpClient client = prepareWithChain(conduit);
+
+ // the certificate constraints are checked for each response, the
client is not built from them,
+ // so changing them, in place or by replacing them, keeps the client
+
tls.getCertConstraints().getSubjectDNConstraints().getRegularExpression().add("CN=two");
+ assertSame(client, prepareWithChain(conduit));
+ TLSClientParameters replaced = new TLSClientParameters();
+ replaced.setCertConstraints(certConstraints("CN=three"));
+ conduit.setTlsClientParameters(replaced);
+ assertSame(client, prepareWithChain(conduit));
+ }
+
+ @Test
+ public void testHttpClientIsNotUsedForHttps() throws Exception {
+ TrustManager[] trustManagers = {new TestTrustManager()};
+ HttpClientHTTPConduit conduit1 = newConduit("http");
+ TLSClientParameters tls1 = new TLSClientParameters();
+ tls1.setTrustManagers(trustManagers);
+ conduit1.setTlsClientParameters(tls1);
+ HttpClient httpClient = prepare(conduit1);
+
+ // a client for http is built without the TLS parameters, so a conduit
for https with
+ // the same parameters must not get it
+ HttpClientHTTPConduit conduit2 = newConduit("https");
+ TLSClientParameters tls2 = new TLSClientParameters();
+ tls2.setTrustManagers(trustManagers);
+ conduit2.setTlsClientParameters(tls2);
+ HttpClient httpsClient = prepare(conduit2);
+ assertNotSame(httpClient, httpsClient);
+ assertNotSame(SSLContext.getDefault(), httpsClient.sslContext());
+
+ // nor the conduit for http itself, when its address changes to https
+ assertNotSame(httpClient, prepare(conduit1,
"https://backend.example/test";));
+ }
+
+ @Test
+ public void testPerRequestSettingsKeepClient() throws Exception {
+ HttpClientHTTPConduit conduit = newConduit("http");
+ HttpClient client = prepare(conduit);
+
+ // settings which are applied to each request, not to the client,
alternate between calls
+ for (int i = 0; i < 4; i++) {
+ HTTPClientPolicy policy = new HTTPClientPolicy();
+ policy.setReceiveTimeout(i % 2 == 0 ? 1000 : 2000);
+ assertSame(client, prepare(conduit, policy, true));
+ conduit.getClient().setReceiveTimeout(i % 2 == 0 ? 3000 : 4000);
+ assertSame(client, prepare(conduit));
+ }
+ }
+
+ @Test
+ public void testAlternatingSettingsKeepOneReferencePerClient() throws
Exception {
+ HttpClientHTTPConduit conduit = newConduit("http");
+ HttpClient client1 = prepare(conduit);
+ conduit.getClient().setConnectionTimeout(1000);
+ HttpClient client2 = prepare(conduit);
+ assertNotSame(client1, client2);
+
+ // switching back and forth between the two settings gets the shared
clients back, and the
+ // conduit does not keep a further reference to them each time
+ for (int i = 0; i < 4; i++) {
+ conduit.getClient().setConnectionTimeout(30000);
+ assertSame(client1, prepare(conduit));
+ conduit.getClient().setConnectionTimeout(1000);
+ assertSame(client2, prepare(conduit));
+ assertEquals(1, deferredClientRefs(conduit).size());
+ }
+ }
+
+ @Test
+ public void testConnectionTimeoutOfMessage() throws Exception {
+ // the connection timeout of the request context is the one the client
is built with
+ Message message = newMessage(true);
+ message.put(Message.CONNECTION_TIMEOUT, 1000);
+ assertEquals(Optional.of(Duration.ofSeconds(1)),
prepare(newConduit("http"), message).connectTimeout());
+
+ // so a conduit without it does not get that client
+ HttpClientHTTPConduit conduit = newConduit("http");
+ assertEquals(Optional.of(Duration.ofSeconds(30)),
prepare(conduit).connectTimeout());
+
+ // and setting it later is not ignored
+ message = newMessage(true);
+ message.put(Message.CONNECTION_TIMEOUT, 1000);
+ assertEquals(Optional.of(Duration.ofSeconds(1)), prepare(conduit,
message).connectTimeout());
+ }
+
+ @Test
+ public void testForcedVersionOfMessage() throws Exception {
+ Message message = newMessage(true);
+ message.put(HTTPConduit.FORCE_HTTP_VERSION, "1.1");
+ assertEquals(HttpClient.Version.HTTP_1_1, prepare(newConduit("http"),
message).version());
+
+ HttpClientHTTPConduit conduit = newConduit("http");
+ assertEquals(HttpClient.Version.HTTP_2, prepare(conduit).version());
+
+ message = newMessage(true);
+ message.put(HTTPConduit.FORCE_HTTP_VERSION, "1.1");
+ assertEquals(HttpClient.Version.HTTP_1_1, prepare(conduit,
message).version());
+ }
+
+ @Test
+ public void testClientsResetForNewHttpsAddressAreNotReused() throws
Exception {
+ HttpClientHTTPConduit conduit = newConduit("https");
+ HttpClient client1 = prepare(conduit, "https://backend1.example/test";);
+ conduit.getClient().setConnectionTimeout(1000);
+ HttpClient client2 = prepare(conduit, "https://backend1.example/test";);
+ assertNotSame(client1, client2);
+
+ // the HTTPS address changes, so neither client may be used again
+ HttpClient client3 = prepare(conduit, "https://backend2.example/test";);
+ assertNotSame(client2, client3);
+ conduit.getClient().setConnectionTimeout(30000);
+ HttpClient client4 = prepare(conduit, "https://backend2.example/test";);
+ assertNotSame(client1, client4);
+ assertNotSame(client3, client4);
+ }
+
+ private static Collection<?> deferredClientRefs(HttpClientHTTPConduit
conduit) throws Exception {
+ Field field =
HttpClientHTTPConduit.class.getDeclaredField("deferredClientRefs");
+ field.setAccessible(true);
+ return (Collection<?>)field.get(conduit);
+ }
+
+ private HttpClientHTTPConduit newConduit(String scheme) throws Exception {
+ EndpointInfo ei = new EndpointInfo();
+ ei.setAddress(scheme + "://backend.example/test");
+ HttpClientHTTPConduit conduit = new HttpClientHTTPConduit(bus, ei,
null);
+ conduit.finalizeConfig();
+ conduits.add(conduit);
+ return conduit;
+ }
+
+ private static HttpClient prepare(HttpClientHTTPConduit conduit) throws
Exception {
+ return prepare(conduit, null, true);
+ }
+
+ private static HttpClient prepare(HttpClientHTTPConduit conduit,
HTTPClientPolicy policy,
+ boolean shareClient) throws Exception {
+ Message message = newMessage(shareClient);
+ if (policy != null) {
+ message.put(HTTPClientPolicy.class, policy);
+ }
+ return prepare(conduit, message);
+ }
+
+ private static HttpClient prepare(HttpClientHTTPConduit conduit, String
address) throws Exception {
+ Message message = newMessage(false);
+ message.put(Message.ENDPOINT_ADDRESS, address);
+ return prepare(conduit, message);
+ }
+
+ private static Message newMessage(boolean shareClient) {
+ Message message = new MessageImpl();
+ message.put(Message.HTTP_REQUEST_METHOD, "GET");
+ if (!shareClient) {
+ message.put("share.httpclient.http.conduit", Boolean.FALSE);
+ }
+ return message;
+ }
+
+ private static HttpClient prepare(HttpClientHTTPConduit conduit, Message
message) throws Exception {
+ conduit.prepare(message);
+ HttpClient client = message.get(HttpClient.class);
+ assertNotNull(client);
+ return client;
+ }
+
+ private static HttpClient prepareWithChain(HttpClientHTTPConduit conduit)
throws Exception {
+ // the certificate constraints add an interceptor to the chain of the
message
+ Message message = newMessage(true);
+ message.setInterceptorChain(new PhaseInterceptorChain(new
TreeSet<>()));
+ return prepare(conduit, message);
+ }
+
+ private static CertificateConstraintsType certConstraints(String subject) {
+ DNConstraintsType constraints = new DNConstraintsType();
+ constraints.getRegularExpression().add(subject);
+ CertificateConstraintsType certConstraints = new
CertificateConstraintsType();
+ certConstraints.setSubjectDNConstraints(constraints);
+ return certConstraints;
+ }
+
+ private static void setProxy(HTTPClientPolicy policy, String host, int
port) {
+ policy.setProxyServer(host);
+ policy.setProxyServerPort(port);
+ }
+
+ private static Proxy proxy(HttpClient client) {
+ List<Proxy> proxies = client.proxy().get().select(TARGET);
+ assertEquals(1, proxies.size());
+ return proxies.get(0);
+ }
+
+ private static String proxyHost(HttpClient client) {
+ Proxy proxy = proxy(client);
+ assertEquals(Proxy.Type.HTTP, proxy.type());
+ return ((InetSocketAddress)proxy.address()).getHostString();
+ }
+
+ private static final class TestTrustManager implements X509TrustManager {
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String
authType) {
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String
authType) {
+ }
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return new X509Certificate[0];
+ }
+ }
+}
