coheigea commented on PR #3673:
URL: https://github.com/apache/cxf/pull/3673#issuecomment-6651741491
@ffang Remaining issues
1. The key still doesn’t use the actual connection timeout or HTTP version
(medium). The client is built from:
- ctimeout = determineConnectionTimeout(message, csPolicy), which is
overridden by Message.CONNECTION_TIMEOUT
(jakarta.xml.ws.client.connectionTimeout, a common request-context property).
- verc, which is overridden by the FORCE_HTTP_VERSION property.
But copyOf and samePolicy use csPolicy.getConnectionTimeout() and
csPolicy.getVersion(). The effects:
- If conduit 1 sets connectionTimeout=1000 in its request context, its
client is built with a 1s timeout but cached under 30s. Conduit 2, with
defaults, then shares that 1s client.
- Later per-request changes to CONNECTION_TIMEOUT on a conduit are silently
ignored.
This bug predates the PR, but it is exactly what the PR now says it
guarantees. Fix: set copy.setConnectionTimeout(ctimeout) and use the effective
verc in the copy.
2. takeDeferredClient can bring back a client that was reset because the
HTTPS host changed (low). The !httpsReset guard only covers the current
request. Example with sharing off:
1. Requests to host A use client X.
2. A request to host B triggers the HTTPS reset, so X is deferred and Y is
built.
3. The connection timeout changes, so Y is deferred and Z is built.
4. The timeout changes back. takeDeferredClient walks the queue oldest-first
and returns X, the client that https.reset.httpclient.http.conduit deliberately
dropped.
Fix: keep HTTPS-reset references out of the reusable set (queue them
separately for release), or at least prefer the newest match. Related, and also
older than this PR: with sharing on, the HTTPS reset barely does anything,
because the cache key doesn’t include the host and computeIfAbsent hands back
the same client.
3. Minor points:
- close() empties deferredClientRefs without holding initializationLock. A
prepare() running at the same time can take a deferred reference back into
clientRef after close() has cleared it, so that reference is never released.
This is the same kind of race that already exists for new clients built during
close, so it’s low priority.
- copyOf(TLSClientParameters) still shares certConstraints by reference.
The new comment explains why that’s fine (the constraints are checked on each
response), but equals still compares them. So changing the constraints in place
forces no rebuild, while replacing them with a different object does. It would
be cleaner to drop them from the client key or copy them.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
