ffang commented on PR #3673: URL: https://github.com/apache/cxf/pull/3673#issuecomment-6639989403
> Thanks, the underlying bug is real. Using the creating conduit’s live policy and TLS parameters as the cache key, and reading that live policy in the ProxySelector, let one conduit’s later changes leak into others. The trust-manager case is especially bad. Copying the settings and removing released clients by identity both look good. > > One concern before merge: the new per-request check if (ref != null && !ref.matches(csPolicy, clientParameters)) resetClient(ref); can leak clients. > > 1. The check compares more than the client is built from. matches uses ClientPolicyCalculator.equals, which includes ReceiveTimeout, Accept*, Cookie, ContentType, ConnectionRequestTimeout and similar. These are per-request settings that HttpClient.Builder never uses. Changing one of them, for example setReceiveTimeout before a call, a message-level HTTPClientPolicy, or WS-Policy that differs per operation, now throws away the conduit’s client and its pooled connections. > 2. Thrown-away clients are only released on close(). resetClient puts the old reference in deferredClientRefs, which is only emptied in close(). A long-lived conduit that alternates between two policies grows that queue without limit. With sharing disabled, or the cache full, each switch builds a new HttpClient with its own selector thread. > > Suggestion: key the cache, and the reset check, only on what the client is built from (connection timeout, proxy host/port/type, non-proxy hosts, version, TLS parameters). Also release a deferred reference once nothing is using it, rather than holding it until close(). A test that alternates a message-level policy with different receive timeouts would cover this. > > Minor: > > 1. copyOf silently falls back to the live object when the copy doesn’t compare equal, which brings the original bug back. Consider at least logging it. > 2. The cipher suite filters and cert constraints are still shared by reference, so changing them after the first call isn’t detected. Thanks @coheigea, good catch. Updated: - The client is now matched only on what `HttpClient.Builder` uses: connection timeout, proxy host/port/type, non-proxy hosts, version and the TLS parameters. Per-request settings (receive timeout, `Accept*`, cookie, ...) no longer replace the client, whether they come from the conduit, a message-level `HTTPClientPolicy` or WS-Policy. - When a conduit switches back to settings it used before, it takes the matching client back from its deferred queue instead of building or acquiring another one. So the queue is bounded by the number of distinct client configurations, not by the number of switches, also with sharing disabled. Releasing a deferred client as soon as it is idle would need a reliable end-of-request point; redirects and retransmissions still use the client after `close(Message)`, and that is why deferred clients are kept until `close()` already today. So I kept that and reused them instead. - `copyOf` no longer falls back to the live objects: the policy copy is built from exactly the compared fields, and the cipher suites filter is now deep-copied. The certificate constraints are still copied by reference: the client is not built from them, they are checked against the conduit's own parameters for each response. - While at it: `TLSClientParameters.equals` didn't compare the hostname verifier, although `SSLUtils` wraps the client's trust managers with it, so conduits which differed only in their hostname verifier shared a client. It is compared now. New tests cover alternating receive timeouts from a message-level policy, alternating connection timeouts with sharing disabled, the hostname verifier, the named groups and a cipher suites filter changed in place. -- 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]
