Add HttpClient5 sampler implementation with HTTP/2 support - #6742
Add HttpClient5 sampler implementation with HTTP/2 support#6742andreaslind01 wants to merge 27 commits into
Conversation
…HTTPHC5Impl` for DNS resolution
…proxies, and authentication
…ng, caching, proxies, and user authentication
…and improve fallback handling with new tests
…TPHC5Impl` and Gradle configurations
…ests for GET and POST requests
…ts, including HTTP/2, with corresponding unit tests
…ers (`Authorization`, `Proxy-Authorization`) and adding unit tests for validation
…ttpClient5 sampler
milamberspace
left a comment
There was a problem hiding this comment.
Not a full review of this PR — just a scoped, timely note on the dependency versions.
Apache HttpComponents Client just released 5.6.4: "Corrects application of SSL parameters in the async TLS upgrade method" (RELEASE_NOTES-5.6.x.txt). This PR pins httpclient5:5.5.1, and HTTPHC5Impl is exactly the kind of code that exercises that path — it uses the async H2 client with HttpVersionPolicy.NEGOTIATE for HTTP/2-over-TLS via ALPN, i.e. an async TLS upgrade. Worth pulling in the fix before this lands, rather than shipping the new HTTP/2 sampler with a known bug in SSL-parameter application during that exact upgrade.
See inline comment for the concrete version bump (and the matching httpcore5 pairing, since httpclient5:5.6.4 is built/tested against httpcore5:5.4.3, not 5.3.4).
This review was drafted by an AI-assisted tool and confirmed by an Apache JMeter maintainer.
| @@ -107,6 +107,8 @@ dependencies { | |||
| because("User might still rely on commons-text") | |||
| } | |||
| api("org.apache.httpcomponents.client5:httpclient5:5.5.1") | |||
There was a problem hiding this comment.
Since HTTPHC5Impl (this PR) exercises HttpClient5's async TLS upgrade path for HTTP/2, worth bumping both of these before merge:
httpclient5:5.5.1→5.6.4— fixes "SSL parameter application in the async TLS upgrade strategy" (release notes)httpcore5/httpcore5-h2:5.3.4→5.4.3— the versionhttpclient5:5.6.4is actually built and tested against (per its parent POM'shttpcore.versionproperty), so bumping onlyhttpclient5and leavinghttpcore5at5.3.4would be an incoherent pairing.
There was a problem hiding this comment.
Thanks — good catch. Bumped httpclient5 to 5.6.4 and httpcore5/httpcore5-h2 to 5.4.3. Confirmed the pairing you flagged: httpclient5-parent-5.6.4.pom sets <httpcore.version>5.4.3</httpcore.version>.
Not a drop-in bump though — it surfaced two real issues:
1. HTTPS/HTTP-2 handshakes broke (SSLHandshakeException: No name matching localhost found). Now that SSL parameters are actually applied on the async path, JSSE endpoint identification runs, and ClientTlsStrategyBuilder defaults to BOTH when a hostnameVerifier is set, silently overriding our NoopHostnameVerifier + TrustAllStrategy. On 5.5.1 that half was a no-op because of the bug. Fixed with .setHostVerificationPolicy(HostnameVerificationPolicy.CLIENT). This would have hit anyone testing HTTPS with a self-signed cert, so good that this landed before the sampler shipped.
2. NoClassDefFoundError in HTTPHC5Impl's static initializer — 5.6 rewrote BrotliInputStreamFactory to use the optional brotli4j, which we don't ship. Fixed by decoding br via org.brotli:dec, already a direct dependency and what HTTPHC4Impl uses.
Also switched deprecated build() → buildAsync() and suppressed the new deprecation warnings (-Werror). Didn't migrate to the suggested ContentCodecRegistry — it's @Internal. Happy to revisit.
classes style clean; :src:protocol:http:test 977 passed / 0 failed.
….4.3 respectively for improved SSL parameter handling in HTTP/2
… and ensuring consistent header reporting across transports
milamberspace
left a comment
There was a problem hiding this comment.
Found a real bug while manually testing the HTTP/2 sampler against a long-running server: intermittent
java.io.IOException: Could not execute HTTP/2 request
Caused by: org.apache.hc.core5.http2.impl.nio.ConnectionClosedException: Connection is closed
at org.apache.hc.core5.http2.impl.nio.H2Streams.shutdownAndReleaseAll(H2Streams.java:149)
at org.apache.hc.core5.http2.impl.nio.AbstractH2StreamMultiplexer.onOutput(...)
...
Root cause
createHttp2Client() never configures ConnectionConfig.validateAfterInactivity on the PoolingAsyncClientConnectionManagerBuilder, and ConnectionConfig.DEFAULT documents it as null (undefined). In PoolingAsyncClientConnectionManager#lease(), the re-validation step (an HTTP/2 PING before handing out a pooled connection, StaleCheckCommand for HTTP/1.1) is gated by:
final TimeValue timeValue = connectionConfig.getValidateAfterInactivity();
if (connection.isOpen() && TimeValue.isNonNegative(timeValue)) { ... }TimeValue.isNonNegative(null) is false, so with the default config this block never runs — pooled async connections are leased straight out of the pool with zero liveness check.
Concretely: HTTP_2_CLIENTS caches the async client per JMeter thread and reuses it across iterations. If the server (or an idle load balancer/NAT) closes an idle pooled HTTP/2 connection between two samples, the next sample picks it from the pool as-is; the I/O reactor only discovers it's dead when it tries to write to it, surfacing as ConnectionClosedException deep in H2Streams.
This is made worse by disableAutomaticRetries() (called on both the classic and async builders) — correctly disabled so JMeter doesn't silently mask real server behavior/timing from the sample result, but it also removes HttpClient5's own safety net for exactly this failure mode. Without proactive pool validation, there's nothing left to catch it, and it surfaces as a hard sampler failure instead of a transparent retry.
Suggested fix
See inline comment — add .setValidateAfterInactivity(...) to the ConnectionConfig built in createHttp2Client() (worth doing for createClient()'s classic-transport config too, same gap applies there).
This review was drafted by an AI-assisted tool and confirmed by an Apache JMeter maintainer.
| if (key.dnsCacheManager != null) { | ||
| connectionManagerBuilder.setDnsResolver(createDnsResolver(key.dnsCacheManager)); | ||
| } | ||
| if (key.connectTimeout > 0) { |
There was a problem hiding this comment.
Worth folding a setValidateAfterInactivity into this ConnectionConfig (unconditionally, not just under the connectTimeout > 0 guard) so pooled HTTP/2 connections get an HTTP/2 PING liveness check before reuse instead of being handed out straight from the pool:
ConnectionConfig.Builder connectionConfig = ConnectionConfig.custom()
.setValidateAfterInactivity(TimeValue.ofSeconds(2));
if (key.connectTimeout > 0) {
connectionConfig.setConnectTimeout(Timeout.ofMilliseconds(key.connectTimeout));
}
connectionManagerBuilder.setDefaultConnectionConfig(connectionConfig.build());Without it, ConnectionConfig.DEFAULT.getValidateAfterInactivity() is null, and PoolingAsyncClientConnectionManager#lease() skips its re-validation step entirely (TimeValue.isNonNegative(null) is false), so a connection the server already closed gets reused as-is and fails mid-write with ConnectionClosedException instead of being transparently discarded and replaced.
|
Following up on the stale-connection bug above with a concrete repro I ran manually against a real server (own domain, browsing-style scenario: 5 threads, 3 loops, ~800–2800ms think time between transactions on the same reused HTTP/2 connection). Worth a regression test to prove Suggested shape for the test:
Happy to be wrong about the exact mechanics of forcing step 3 cleanly with whatever test HTTP/2 server this project already has infrastructure for ( |
…lures from closed connections
|
Thanks @milamberspace - fixed and covered by a regression test. Fix: One correction on the mechanics, which changed how the test had to be shaped. Tracing httpclient5 5.6.4 / httpcore5 5.4.3: a cleanly closed connection isn't what fails. What the test does instead, keeping the idle-gap framing: A raw TCP relay sits in front of WireMock's h2/TLS port, so the test owns the client-facing socket. It can mark a connection doomed: it stays open, but is dropped as soon as the client writes to it again - what an idle server/LB timeout looks like to a client that hasn't noticed yet.
On asserting it fails without the fix: verified directly by forcing the default to
|
Description
This PR adds a new HTTP sampler implementation,
HttpClient5, based on Apache HttpComponents HttpClient 5.x, and introduces a configurable HTTP Version setting (HTTP/1.1/HTTP/2) for both theHttpClient5and theJavaimplementation.Main changes:
HTTPHC5Impl(HTTPSamplerFactory.IMPL_HTTP_CLIENT5, selectable asHttpClient5in the GUI and in JMX files):HttpVersionPolicy(FORCE_HTTP_1/NEGOTIATE), with automatic fallback to HTTP/1.1 when the server does not offer h2 via ALPN.AuthManager(BASIC/DIGEST, pre-emptive BASIC),CacheManager(conditional requests viaIf-Modified-Since/If-None-Match),CookieManager,DNSCacheManager, response decompression (gzip/deflate/brotli), and retry handling.SampleResultmetrics:sentBytes,connectTime(measured for both HTTP/1.1 and HTTP/2, including TLS),latency, headers and response code/message.HTTPJavaImpl: HTTP/2 support via the JDKjava.net.http.HttpClientwhenHTTP/2is selected, including caching, proxies, user authentication,sentBytesaccounting, connect-time measurement, reason-phrase derivation (HTTP/2 has no reason phrase) and preservation ofAuthorization/Proxy-Authorizationheaders.HTTPSampler.httpVersion(HTTPSamplerBaseSchema.httpVersion, getter/setter onHTTPSamplerBase) with a new combo box in HTTP Request and HTTP Request Defaults (http_versionresource key added to allmessages_*.properties).CacheManager: new overloads for HC5 (ClassicHttpRequest/ClassicHttpResponse/org.apache.hc.core5.http.Header[]) and for the JDKjava.net.http.HttpResponse.httpclient.versionre-purposed as the default HTTP version (HTTP/1.1|HTTP/2) used when the sampler's HTTP Version field is empty.httpclient5andhttpcore5added tosrc/protocol/httpand to the third-party BOM (httpcore5:5.3.4).component_reference.xml,properties_reference.xml,get-started.xml,bin/jmeter.properties.Motivation and Context
JMeter's HTTP samplers currently only support HTTP/1.1: the
HttpClient4implementation is built on the HttpComponents 4.x line, which will not receive HTTP/2 support, and theJavaimplementation used the legacyHttpURLConnection. Modern web applications and APIs are increasingly served over HTTP/2, so load tests against them either could not be executed at all or did not represent realistic client behaviour (multiplexing, HPACK header compression, single connection per origin).This change gives users a supported migration path to HttpComponents 5.x and makes it possible to run load tests over HTTP/2 — either with the fully featured
HttpClient5implementation or, for lightweight scenarios, with the JDK client in theJavaimplementation. Existing test plans are unaffected:HttpClient4remains the default and an empty HTTP Version falls back to the previous HTTP/1.1 behaviour.Fixes:
How Has This Been Tested?
TestHTTPHC5Features(16 tests): version selection and precedence (sampler value vs.httpclient.versionvs. unsupported value), HTTP/2 usage, fallback to HTTP/1.1 when the server does not support h2, HTTP/2 via proxy,sentBytesfor GET/POST, conditional requests throughCacheManager, BASIC credentials fromAuthManager, proxy authentication, andconnectTimefor HTTP/1.1 and HTTP/2.TestHTTPJavaFeatures(~16 tests): version selection, HTTP/2 requests (incl. via proxy), response message / reason-phrase handling for HTTP/2,sentBytesfor GET/POST in both versions,Authorizationheader from theHeaderManager, andconnectTimefor HTTP/1.1, HTTP/2 plaintext and HTTP/2 over TLS.TestHTTPSamplerFactory: creation and lookup of the newHttpClient5implementation, plus the unchanged behaviour for the existing aliases../gradlew classes style— compiles cleanly and reports no style/checkstyle/autostyle violations.src:protocol:httptest suite (includingJMeterTest, extended byhttpVersionin the ignored-properties list) still passes.HTTP/2in the View Results Tree.JMeter 6.0.0-SNAPSHOT.Screenshots (if appropriate):
Types of changes
Checklist: