ffang commented on PR #3673:
URL: https://github.com/apache/cxf/pull/3673#issuecomment-6697715422
> @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:
>
> * Requests to host A use client X.
> * A request to host B triggers the HTTPS reset, so X is deferred and Y is
built.
> * The connection timeout changes, so Y is deferred and Z is built.
> * 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.
Thanks @coheigea, all addressed (I also rebased on main, so the first two
commits have new hashes):
1. **Effective connection timeout and version:** fixed in 67c01db170. The
key and the reset check now use the connection timeout and HTTP version in
effect for the request, after `Message.CONNECTION_TIMEOUT` and
`FORCE_HTTP_VERSION` are applied, not the policy's values. So a conduit no
longer shares a client built with another conduit's request-context timeout,
and it picks up its own per-request changes.
2. **Taking back an HTTPS-reset client:** fixed in 67c01db170. Clients
dropped because of the HTTPS reset, and any deferred before it, now go to a
separate queue that is only released on `close()`. Only clients replaced
because the settings changed can be taken back, so in your example X is never
handed out again.
I left the related point alone: with sharing on, the HTTPS reset barely
does anything because the cache key doesn't include the host. That predates
this PR and changes how sharing works, so I'd rather handle it in a follow-up
issue.
3. **Minor points:**
- `close()` now takes the current, reusable and deferred references under
`initializationLock`, the same lock `takeDeferredClient` runs under. A
concurrent `prepare()` can no longer take a deferred reference back after
`close()` has collected them.
- The certificate constraints are no longer part of the client key. The
client isn't built from them, and they're checked on each response, so changing
them in place or replacing them keeps the client. The conduit's copy doesn't
keep them.
While testing this I also found that a client built for an `http` address
was keyed on the TLS parameters, even though it's built without them and uses
the default `SSLContext`. An `https` conduit with the same TLS parameters, or
the same conduit after its address changed to `https`, could then reuse it
without its own trust managers, key managers, cipher suites or protocols.
c64fb71cd5 keys such clients without TLS parameters.
The settings copy now uses the new `TLSClientParameters.copy()`, as @reta
suggested (05ae30fc43).
--
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]
