Add HttpClient5 sampler implementation with HTTP/2 support - #6742
Add HttpClient5 sampler implementation with HTTP/2 support#6742andreaslind01 wants to merge 59 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
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
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
vlsi
left a comment
vlsi
left a comment
There was a problem hiding this comment.
Review comments — PR #6742 "Add HttpClient5 sampler implementation with HTTP/2 support"
Reviewed at head 919b9125, against merge base ad6ecbd1.
Two independent reviewers (Codex and a Claude subagent) went through the diff; the items below are the merged, verified
result. Line numbers refer to the PR head.
Blockers
B1. HTTPHC5Impl ends the sample before the body is read, so elapsed excludes the download and latency exceeds elapsed
HTTPHC5Impl.java:444-448 calls result.sampleEnd() right after executeRequest() returns. For the classic transport
that point is "status line and headers received" — the body is read afterwards, in updateResult() →
readResponse() (HTTPHC5Impl.java:904). HTTPSamplerBase.readResponse() calls sampleResult.latencyEnd() on the
first body byte (HTTPSamplerBase.java:1972), which now happens after endTime was stamped.
GET a 100 MB file over HTTP/1.1 with HttpClient5: headers arrive after 50 ms, the body takes 10 s. The sample reports
elapsed ≈ 50 ms and latency ≈ 10050 ms. Elapsed, throughput, the Latency column, the aggregate report and the HTML
dashboard are all wrong, and latency > elapsed is a state JMeter's own model treats as impossible.
HTTPHC4Impl does the opposite order deliberately — HTTPHC4Impl.java:667-672, res.sampleEnd(); // Done with the sampling proper. The new HTTPJavaImpl HTTP/2 path also gets it right, so HC5 is the odd one out.
Suggested fix: read the entity inside the try block, then sampleEnd(), then fill in status code, headers, sizes and
redirect location. Add a regression test asserting result.getLatency() <= result.getTime() against a WireMock stub
with withChunkedDribbleDelay.
B2. The HttpClient5 HTTP/1.1 transport ignores SSLManager: no trust-all, no client keystore, no https.socket.protocols
HTTPHC5Impl.createClient() (HTTPHC5Impl.java:1043-1066) builds the classic connection manager without installing any
TLS socket strategy, so HttpClient falls back to SSLContexts.createSystemDefault(). JMeter's per-thread SSLContext
from JsseSSLManager — which carries the https.keyStore client certificates, the trust-all trust manager,
https.socket.protocols and https.cipherSuites — is never used, and HttpClient 5.4+ defaults
HostnameVerificationPolicy to BOTH.
Since an empty HTTP Version maps to FORCE_HTTP_1, this is the default path. Three failures follow:
- an HTTPS target with a self-signed or internal-CA certificate fails with
SSLHandshakeException, while the same plan
works withHttpClient4andJava; - client-certificate authentication configured through the JMeter keystore silently sends no certificate;
https.socket.protocolsandhttps.cipherSuiteshave no effect.
Note the asymmetry inside the same class: the async client does install a TLS strategy
(HTTP_2_TLS_STRATEGY, HTTPHC5Impl.java:391-405), so the same URL behaves differently depending on the selected HTTP
version. HTTPHC4Impl installs LazyLayeredConnectionSocketFactory (HTTPHC4Impl.java:1106), which wraps
HttpSSLProtocolSocketFactory → JsseSSLManager.
No test covers this: every HTTPS test in TestHTTPHC5Features sets setHttpVersion("HTTP/2") and routes to the async
client.
Suggested fix: build one TLS strategy from ((JsseSSLManager) SSLManager.getInstance()).getContext() plus the two
cipher/protocol properties and NoopHostnameVerifier, and install it on both connection managers
(setTlsSocketStrategy / setTlsStrategy). Resolve the SSLContext lazily per client so the per-thread context and
resetContext() keep working.
B3. createHttp2TlsStrategy() builds a trust-all context and drops the configured client certificate
HTTPHC5Impl.java:391-405 always creates a fresh SSLContexts.custom().loadTrustMaterial(null, TrustAllStrategy…)
context. Nothing from JsseSSLManager reaches it, so HTTP/2 requests to a mutual-TLS endpoint present no client
certificate and fail TLS authentication, unlike every existing implementation.
This is the same root cause as B2 seen from the other side: there is one JMeter SSLContext and neither transport uses
it. Fixing B2 and B3 together with a single shared strategy is the right shape.
B4. HTTPJavaImpl now sends every Header Manager header twice on the HTTP/1.1 path
setupConnection() already calls setConnectionHeaders(conn, u, getHeaderManager(), getCacheManager())
(HTTPJavaImpl.java:440). The diff adds a second call in sample() (HTTPJavaImpl.java:816), and
setConnectionHeaders uses conn.addRequestProperty(n, v), which appends rather than replaces.
A plan on the legacy Java implementation with a Header Manager entry Accept: application/json now puts
Accept: application/json,application/json on the wire. Cookie, Range and Authorization are duplicated the same
way. It is silent, because res.setRequestHeaders(...) is computed inside setupConnection before the second call, so
the sample result still shows one copy.
This regresses the existing HTTP/1.1 Java sampler for everyone, and the only reason for the second call is to obtain
securityHeaders for the new calculateSentBytes.
Suggested fix: return the security headers from setupConnection (or hold them in a field) instead of re-running
setConnectionHeaders. Add a WireMock test asserting the header arrives exactly once.
B5. SHARED_HTTP_2_CLIENTS grows without bound: keyed by a per-thread SSLContext, never evicted, never closed
getHttpClient(URL) puts the SSLContext returned by ((JsseSSLManager) SSLManager.getInstance()).getContext() into
the cache key, and SSLContext does not override equals/hashCode, so the key compares by identity.
JsseSSLManager.getContext() returns a per-thread context by default (https.sessioncontext.shared defaults to false).
So with the default http.java.h2.multiplexing=true, the static SHARED_HTTP_2_CLIENTS map holds one
java.net.http.HttpClient per JMeter thread rather than one shared client — the stated multiplexing goal is not met —
and each client owns a selector thread and a connection pool.
It gets worse across iterations: HTTPHC5Impl/HTTPHC4Impl call resetContext() on every thread-group iteration when
"same user on next iteration" is off (HTTPHC5Impl.java:1281). The next HTTPS HTTP/2 sample then gets a new
SSLContext, a new map key and a new HttpClient, while the old one stays in the static map forever.
200 threads × 500 iterations against an HTTPS target accumulates up to 100 000 HttpClient instances and their selector
threads. The JVM dies with OutOfMemoryError: unable to create native thread.
Nothing ever removes entries — there is no threadFinished()/testEnded() hook for these maps — and HTTP_2_EXECUTOR
(an unbounded cached pool) is never shut down either. HTTPHC5Impl does clean up in threadFinished(); HTTPJavaImpl
does not.
Suggested fix: keep the SSLContext instance out of the key, add a threadFinished()/testEnded() hook that closes
the clients and shuts the executor down, and add a test that samples HTTPS twice with a resetContext() in between and
asserts the map does not grow.
B6. The release-jar manifest is not updated, so :src:dist:verifyReleaseDependencies fails
src/dist/src/dist/expected_release_jars.csv has no entries for httpclient5, httpcore5 or httpcore5-h2, and the
PR does not touch the file. CI already reports it:
External dependencies differ (you could update
src/dist/src/dist/expected_release_jars.csvif you run
:src:dist:verifyReleaseDependencies -PupdateExpectedJars)
Run that task and commit the result. Please also confirm the new jars carry the license and NOTICE coverage the ASF
release requires.
Major
M1. Neither HTTP/2 path can be interrupted, so "Stop Test Now" leaves threads blocked
HTTPHC5Impl.interrupt() (HTTPHC5Impl.java:1304-1312) cancels currentRequest, but the request actually executed
over HTTP/2 is a copy — SimpleHttpRequest asyncRequest = SimpleHttpRequest.copy(request)
(HTTPHC5Impl.java:1142). The classic request never becomes the cancellable dependency of the async exchange, so
cancel() is a no-op while the sampler thread sits in responseFuture.get(...).
HTTPJavaImpl has the mirror problem: interrupt() only touches savedConn, which sampleHttp2() never assigns, so
it returns false and the thread stays blocked in client.httpClient.send(...).
Against a server that accepts the connection and never answers, an HC5 HTTP/2 sampler with no Response Timeout blocks
for the hard-coded 60 s of getHttp2ExecutionTimeoutMillis() and a Java HTTP/2 sampler blocks forever. Neither
"Stop Test Now" nor the JMeter thread interrupt shortens it.
Suggested fix: hold the Future in a volatile field and cancel it from interrupt(); for HTTPJavaImpl, use
sendAsync and keep the CompletableFuture. While you are there, drop the arbitrary 60 s ceiling — HttpClient enforces
the configured response timeout itself, and a legitimate 90-second sample currently fails at 60 s on HTTP/2 but succeeds
on HTTP/1.1.
M2. Both HTTP/2 paths buffer the whole request and response body in heap
HTTPHC5Impl.executeHttp2() converts the request entity with EntityUtils.toByteArray(requestEntity)
(HTTPHC5Impl.java:1144-1149) and executes a SimpleHttpRequest, whose SimpleHttpResponse holds the entire body as a
byte array; createClassicResponse() then wraps that array in a ByteArrayEntity — a third copy.
HTTPJavaImpl.sampleHttp2() has the same shape on the request side: CapturingHttpURLConnection.getOutputStream()
returns a ByteArrayOutputStream, so sendPostData/sendPutData write the whole upload into heap before
BodyPublishers.ofByteArray(...) is built.
Two concrete failures: a POST that uploads a 2 GB file with Files Upload dies with OutOfMemoryError on both HTTP/2
paths while it succeeds on HTTP/1.1; and downloading a 1 GB response over HTTP/2 keeps the full gigabyte in heap even
when httpsampler.max_bytes_to_store_per_request is set, because that limit is applied later, inside
HTTPSamplerBase.readResponse, when the bytes are already materialized. Under load this multiplies by the thread count.
Suggested fix: use the streaming async API — AsyncRequestBuilder with a FileEntityProducer/BasicRequestProducer,
and an AbstractBinResponseConsumer that feeds HTTPSamplerBase.readResponse's truncation logic. For HTTPJavaImpl,
publish with BodyPublishers.ofFile/ofInputStream.
M3. HTTPHC5Impl never updates the sample URL after an automatic redirect
The request config enables HttpClient's own redirect handling (HTTPHC5Impl.java:520), but sample() never writes the
final URI back into the result — there is no result.setURL(...) anywhere in the class. HTTPHC4Impl does exactly that
(HTTPHC4Impl.java:708-718).
Consequences:
saveConnectionCookies(response, result.getURL(), getCookieManager())runs with the original URL, so aSet-Cookie
issued by the redirect target is stored against the original host. A login flow
http://www.example.com/login→https://auth.example.com/sessionstores the session cookie forwww.example.com,
and every later request is unauthenticated.cacheManager.saveDetails(response, result)usesres.getUrlAsString(), so the ETag of the redirect target is cached
under the original URL.- Listeners show the pre-redirect URL.
The same method also drops a redirect without a Location header silently, where HC4 raises IllegalArgumentException
(HTTPHC4Impl.java:684-686).
M4. ConnectTimeTracker stamps connectEnd() onto unrelated in-flight samples
ConnectTimeTracker.recordConnectEnd() walks all entries of activeSamples and calls result.connectEnd() on each.
The tracker belongs to an Http2Client, and with http.java.h2.multiplexing=true that client lives in the static
SHARED_HTTP_2_CLIENTS map — one instance for every JMeter thread and every origin, since HttpClientKey carries no
host or port.
Thread A samples https://a.example over an already-pooled connection while thread B opens a new connection to
https://b.example; B's TLS handshake stamps a connect time into A's result.
Separately, ConnectTimeMeasuringExecutor.execute() calls tracker.connectionEstablished() on every task the JDK
client submits — body delivery, stream callbacks, retries — not only on connection establishment, so the first such task
after a sample starts is reported as its connect time even on a fully pooled connection.
recordsConnectTimeForEveryMultiplexedSample (TestHTTPJavaFeatures.java:219-242) asserts this behavior rather than the
invariant, so it locks the defect in. There is also a cross-thread write to a SampleResult owned by another thread.
Suggested fix: scope the tracker to a connection rather than to a client, drop the executor heuristic, and change the
test to assert that a sample served by an established connection reports connectTime == 0.
M5. HTTPHC5Impl reports the decompressed length as bodySize
updateResult() sets bodySize = body.length where body is the decoded payload
(HTTPHC5Impl.java:901-914). HTTPHC4Impl derives it from connection metrics —
res.setBodySize(totalBytes - headerBytes) — that is, wire bytes; and the new HTTPJavaImpl HTTP/2 path uses a
CountingInputStream around the compressed stream.
A gzip response of 20 KB on the wire that expands to 400 KB is reported as 400 KB by HttpClient5 and as 20 KB by
HttpClient4 and Java. Switching a plan's implementation changes the Bytes column and any bandwidth SLA by a factor of
20, with no warning. headersSize is approximated differently too.
Suggested fix: wrap the raw entity stream in org.apache.jorphan.io.CountingInputStream before the decompressing
wrapper — the class HTTPJavaImpl already uses — and compute headersSize the way HC4 does. TestDecompression is
already parameterized across all three implementations; an assertion on getBytesAsLong() there would cover it.
M6. HTTPJavaImpl sent-bytes counts the human-readable body preview for file uploads
The HTTP/1.1 path computes postBodyBytes = getBytes(postBody) where postBody is the String returned by
sendPostData/sendPutData — the display form JMeter puts in the Request Body tab. For a file upload that is the
placeholder <actual file content, not shown here> plus the multipart boundary preview, not the payload.
calculateSentBytes then takes the postBodyBytes.length > 0 branch and never falls back to Content-Length.
POST a 10 MB file with Files Upload and the sample reports a few hundred sent bytes — the sent-bytes graph and the
dashboard understate upload traffic by three orders of magnitude.
The requestHeaders snapshot for the same computation is taken from conn.getRequestProperties() before
setPostHeaders/setPutHeaders has added Content-Length/Content-Type, so the header portion is under-counted too.
Suggested fix: count what is actually written by wrapping conn.getOutputStream() in a counting stream, falling back to
Content-Length.
M7. HTTPJavaImpl mutates global system properties from a static initializer
HTTPJavaImpl has static { applyHttp2SystemProperties(JMeterUtils.getJMeterProperties(), System.getProperties()); },
which writes jdk.httpclient.hpack.maxheadertablesize, jdk.httpclient.maxstreams, jdk.httpclient.windowsize,
jdk.httpclient.connectionWindowSize, jdk.httpclient.maxframesize, jdk.httpclient.keepalive.timeout.h2 and —
unconditionally — jdk.httpclient.enablepush into the JVM-wide table, as a side effect of loading the class.
Selecting the legacy Java implementation for a plain HTTP/1.1 sampler, or running TestDecompression (which
instantiates every implementation), flips jdk.httpclient.enablepush to 0 for every other component in the JVM:
JSR223 scripts, plugins, the backend listener, anything that builds its own java.net.http.HttpClient. In a
distributed-test server process the change survives across runs and cannot be undone. The setUnlessDefined guard only
protects values that are already present.
JMeter already has system.properties for exactly this. Suggested fix: apply the properties lazily and idempotently
when the first HTTP/2 sample is about to be taken, skip jdk.httpclient.enablepush when the JMeter property is absent,
or document that these belong on the command line and drop the mutation.
Design and reuse
D1. HTTPHC5Impl re-implements HTTPHC4Impl rather than extracting the shared logic
Roughly 700 of HTTPHC5Impl's 1499 lines are HTTPHC4Impl with the types substituted: getRequestHeaders,
getAllHeadersExceptCookie, setConnectionCookie, saveConnectionCookies, getOnlyCookieFromHeaders,
setConnectionHeaders, setupRequestEntity/createNameValuePairs, HttpClientKey (12 fields plus equals/
hashCode), the Kerberos SPN and strip-port logic, the route planner, and the connect-time-measuring connection
manager. CountingOutputStream (HTTPHC5Impl.java:997-1013) is derived a third time inside HTTPJavaImpl's
calculateSentBytes.
This is not a theoretical cost: B1, M3 and M5 are all cases where the HC5 copy drifted from the HC4 original within a
single PR. Lifting the cookie, entity and sent-bytes helpers into HTTPHCAbstractImpl (or a package-private utility
shared by HC4 and HC5) would have made those three impossible.
D2. CacheManager grows a third and fourth parallel API
CacheManager now carries three near-identical saveDetails overloads (HC4 HttpResponse, HC5 ClassicHttpResponse,
java.net.http.HttpResponse), two inCache, two setHeaders and two asHeaders adapters. A fix to the Vary or
cache-control handling now has to land in three places, and a core config element acquires a compile-time dependency on
both HttpClient 4 and HttpClient 5 — which also pins those versions into JMeter's public API for plugin authors.
Suggested shape: one saveDetails(ResponseHeaderSource, HTTPSampleResult), one inCache(URL, Header[]) and one
setHeaders(URL, RequestHeaderSink), with the adapters at the three call sites. The existing
CacheManager.Header/HeaderAdapter pair is already most of that abstraction.
D3. Is HTTPJavaImpl the right home for a second full HTTP/2 client?
HTTPJavaImpl grows from ~350 to ~1800 lines. Around 250 of those are a hand-written delegating SSLContext /
SSLContextSpi / SSLEngine triple whose only purpose is to observe when the TLS handshake finished, and ~60 more are
a hand-maintained IANA reason-phrase table (HTTP_REASON_PHRASES) — EnglishReasonPhraseCatalog is already on this
module's classpath.
The legacy Java implementation exists as the minimal, dependency-free fallback. Putting a second production HTTP/2
stack inside it means two independent HTTP/2 implementations to maintain, with different bugs (compare M4 against the
HC5 connect-time path) and different semantics (equals vs equalsIgnoreCase on the same stored value, see N5).
Worth deciding explicitly before merge: either HTTPJavaImpl gets HTTP/2 as a thin java.net.http passthrough without
the connect-time instrumentation and the multiplexing machinery, or the HTTP/2 support lives only in HTTPHC5Impl and
this PR shrinks by about a third. Splitting the PR along that line would also make it reviewable — 4855 lines across two
new HTTP stacks, a new GUI component, new core API and a dependency bump is a lot to land at once.
D4. HTTP/2 multiplexing across virtual users changes what the test measures
http.java.h2.multiplexing and httpclient5.h2.multiplexing both default to true, and for the Java implementation
that means one HttpClient shared by every JMeter thread. A load test then drives N virtual users over a single TCP
connection, which is not what N real browser users do — connection-level effects (congestion window, server-side
per-connection limits, TLS handshake cost) disappear from the measurement.
Multiplexing within one thread's embedded-resource downloads is clearly right. Sharing across threads is a modeling
decision that deserves a default of false, or at least an explicit note in component_reference.xml about what it
does to the results.
Minor and nits
N1. Response stream leaked when obey_contentlength short-circuits
In readResponse(HttpResponse<InputStream>, SampleResult) the if (contentLength == 0 && OBEY_CONTENT_LENGTH) branch
returns NULL_BA without closing in (response.body()). The JDK client releases an HTTP/2 stream only once the body
stream is consumed or closed, so with httpsampler.obey_contentlength=true against a target that answers
Content-Length: 0 every sample leaks a stream. Restructure as try-with-resources around response.body().
N2. Test quality
Several of the 1756 new test lines cannot fail when the behavior they name breaks:
enablesMessageMultiplexingWithoutRequiringHttpClient55,doesNotRequireProtocolUpgradeConfigurationand
doesNotRequireHttpAsyncClassicAdapterreadHTTPHC5Impl.classbytes and run a 90-line hand-written constant-pool
parser (TestHTTPHC5Features.java:784-846). They assert on the shape of the compiled artifact, throw
IOException("Unknown class-file constant-pool tag")on any tag a future JVM adds, and would pass if multiplexing
were looked up reflectively but never invoked.appliesHttp2ProtocolSettingsasserts the literals 8192/250/65535/65536, which are httpcore5's ownH2Config
defaults — it re-asserts the library, breaks on a dependency bump, and never covers the JMeter property override it
exists for.setsSentBytesCorrectlyForGetRequestasserts onlygetSentBytes() > 0, and the POST variant only
> "hello world".length(), so the sent-bytes arithmetic this PR adds is effectively untested. Assert the exact count
against WireMock's request journal.multiplexesConcurrentRequestsOverASingleHttp2Connection(both copies) asserts four HTTP/2 200s and would pass with
four separate connections. Count accepted connections instead.TestHTTPHC5FeaturessleepsvalidateAfterInactivityMillis() + 500(2.5 s) andThread.sleep(50); neither suite
closes the static clients it creates, so I/O reactor and selector threads accumulate for the life of the test JVM.
Related: the reflective findMessageMultiplexingSetter() lookup and its "JMeter still runs with older HttpClient
versions" comment are dead weight — build.gradle.kts pins httpclient5 at 5.6.4, so the method is always present.
Dropping the reflection removes the three class-file tests with it.
N3. HttpVersionComboBox hard-codes an English label next to a localized one
HttpVersionComboBox.java:47 defines "HTTP/2 Negotiate" as a Java constant with $NON-NLS-1$, while the label beside
it goes through JMeterUtils.getResString("http_version") and was translated into all 12 bundles. A user running JMeter
in German or Japanese sees a translated field label followed by an untranslated value, and translators have no key.
component_reference.xml documents the string as user-visible, so it is UI text.
Also, http_version was inserted out of alphabetical order in all 12 bundles (messages.properties:476 sits between
html_assertion_title and html_report; messages_de.properties:19 sits right after about).
N4. HttpDefaultsGui writes httpVersion unconditionally
HttpDefaultsGui.java:157 does config.set(httpSchema.getHttpVersion(), String.valueOf(httpVersion.getSelectedItem()))
while HttpTestSampleGui.java:183-185 normalizes a blank selection to null. Every HTTP Request Defaults element
opened and saved in the GUI therefore grows an empty HTTPSampler.httpVersion property — spurious diffs in
version-controlled JMX files — and String.valueOf turns a null selection into the literal "null". Neither GUI resets
the combo in clearGui().
N5. Duplicated constants and inconsistent version matching in the new API
HTTPConstantsInterfacegains bothHTTP_VERSION_2 = "HTTP/2"andHTTP_2 = "HTTP/2"— two public constants with
identical values and overlapping Javadoc. Callers already mix them.HTTPSamplerBase.HTTP_VERSION = "HTTPSampler.httpVersion"shadowsHTTPHCAbstractImpl.HTTP_VERSION = JMeterUtils.getPropDefault("httpclient.version", "1.1")by simple name inside the same hierarchy: unqualified
HTTP_VERSIONmeans the property value in one class and the property name in the other. The rest of
HTTPSamplerBasenow derives such constants from the schema —HTTPSamplerBaseSchema.INSTANCE.getHttpVersion() .getName()would keep that convention.HTTPHC5Impl.getHttpVersionPolicycomparesHTTP_VERSION_2_STRICTwithequalsIgnoreCasebutHTTP_VERSION_2with
equals, sohttp/2selects HTTP/1.1 whilehttp/2 strictselects HTTP/2.HTTPJavaImpl.isHttp2uses
equalsIgnoreCasefor both, so the two implementations disagree about the same stored value.
N6. calculateSentBytes uses the platform default charset and models HTTP/2 as HTTP/1.1 text
Every getBytes(Charset.defaultCharset()) in HTTPHC5Impl.calculateSentBytes (HTTPHC5Impl.java:957-976) measures
header names and values in the platform encoding rather than the ISO-8859-1 the wire format uses, so the same plan
reports different sent bytes on a non-UTF-8 JVM.
More fundamentally, the method reconstructs an HTTP/1.1 request line and CRLF-separated headers even when the request
went out over HTTP/2, where headers are HPACK-compressed pseudo-headers — the reported value is systematically too high
once HPACK indexing kicks in on the second request to a host. HTTPHC4Impl takes the real count from connection
metrics (HTTPHC4Impl.java:701); HC5 exposes the same through HttpClientContext.getEndpointDetails() .getSentBytesCount().
Also: for a repeatable entity the method calls entity.writeTo(counter), which re-reads the whole upload from disk on
every sample purely to count it.
N7. httpclient.version is re-purposed but two existing readers still expect the old values
bin/jmeter.properties and properties_reference.xml now document httpclient.version as taking HTTP/1.1, HTTP/2
or HTTP/2 Strict. Two existing readers still expect 1.0/1.1:
AjpSampler.java:180—JMeterUtils.getPropDefault("httpclient.version","1.1").equals("1.0");HTTPHCAbstractImpl.java:80—protected static final String HTTP_VERSION = JMeterUtils.getPropDefault( "httpclient.version", "1.1").
So the documentation is now wrong for AJP, and a user who set httpclient.version=1.1 for the old meaning gets an
unrecognized value for HC5/Java. Either introduce a separate property for the new domain, or accept both spellings and
say so in the docs.
N8. Documentation gaps
changes.xmlnever announces the headline feature. All seven entries describe HTTP/2 details and assume
HttpClient5already exists; there is no entry for the new sampler implementation itself or for the new
HTTPSampler.httpVersionproperty.- The HTTP Request Defaults
<properties>block incomponent_reference.xmlgains noHTTP Versionentry, although
HttpDefaultsGuiadds the field. <dt><code>HTTPClient5</code></dt>(component_reference.xml:145) uses casing that does not match the alias
HttpClient5— it mirrors the pre-existingHTTPClient4typo, so fixing both is optional but welcome.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…HC5Impl samplers, including interrupt handling and corresponding tests
…ownloads Both HTTP/2 sampler paths materialized entire bodies in heap: HTTPHC5Impl converted the request entity with EntityUtils.toByteArray() and held the response as a byte array (copied again into a ByteArrayEntity), while HTTPJavaImpl captured uploads into a ByteArrayOutputStream before building BodyPublishers.ofByteArray(). Large uploads died with OutOfMemoryError where HTTP/1.1 succeeded, and httpsampler.max_bytes_to_store_per_request could not help downloads because it is applied only after the bytes exist. Requests now stream: HTTPHC5Impl uses AsyncRequestBuilder with a FileEntityProducer over a new memory-first SpillOutputStream (256 KB in heap, then a temp file), and HTTPJavaImpl publishes with BodyPublishers.ofFile. Direct file uploads stream with no copy at all via PathEntity. Responses now truncate at the source: the async response consumer stores only max_bytes_to_store_per_request bytes while still counting the full body, so the bytes beyond the limit never reach the heap. Truncation is skipped when the body is content-encoded (a partial gzip stream cannot be decoded) and when MD5 or recording needs the whole body; the limit is resolved on the sampler thread because the I/O reactor threads cannot see JMeter's ThreadLocal context. That gives the same peak heap usage as the HTTP/1.1 transport, which reads and truncates the body as it arrives. Separately, HTTP/2 downloads were about three times slower than the same request over HTTP/1.1 or with the Java implementation, because httpclient5.h2.initial_window_size defaulted to the 64 kB minimum the HTTP/2 specification mandates. The stream flow control window caps the throughput of a response at the window divided by the round trip time, no matter how much bandwidth is available. JMeter now announces the same 16 MB window the JDK client uses, which is already the default of the Java implementation. On a 100 MB download this cut the time from 15 s to 4 s. The connection level window needs no configuration, as HttpCore maximizes it on its own. Also fixed along the way: * multipart previews materialized the whole uploaded file in heap * response bodies were dropped when the server sent no Content-Type * gzip HTTP/2 responses were not decoded * spilled upload files were deleted between internal redirect attempts * setBodySize() reported the truncated length rather than the wire length Extract Http2CapturingHttpURLConnection from HTTPJavaImpl and add SpillOutputStream; commons-io is not a dependency of this module.
…s and ensure location header presence
…meTracker to measure connection establishment duration and update sample results accordingly
… size and response bytes accounting across HTTP implementations. Update tests to validate compressed body size handling.
…oduce CountingOutputHttpURLConnection to track request body size accurately. Update sent bytes calculation and add Content-Length header when missing. Add tests for large file uploads.
- Remove eager static initializer in `HTTPJavaImpl` that mutated global JVM system properties on class loading. - Apply HTTP/2 `jdk.httpclient.*` system properties lazily and idempotently when the first HTTP/2 client instance is created. - Skip setting `jdk.httpclient.enablepush` when `http.java.h2.push_enabled` is not explicitly defined in JMeter properties, preserving JDK defaults for other JVM components. - Update `TestHTTPJavaFeatures` to verify JDK defaults remain intact when properties are unset.
…th is enabled Wrap the HTTP/2 response body stream in a try-with-resources block in HTTPJavaImpl#readResponse(HttpResponse<InputStream>, SampleResult). Previously, when httpsampler.obey_contentlength was set to true and the server returned Content-Length: 0, the method returned early without closing the response stream, leaking JDK HTTP/2 streams.
Replace tests that could not fail with ones that pin down the behaviour they name: - Drop the reflective setMessageMultiplexing() lookup, httpclient5 is pinned at 5.6.4, and remove the three tests that parsed the compiled class file to check for it. - Assert the exact sent bytes against WireMock's request journal. This uncovered that calculateSentBytes() ignored the headers the protocol layer writes itself, so Host, Content-Type, Content-Encoding and Content-Length/Transfer-Encoding are now accounted for as well. - Count the accepted TCP connections in both multiplexing tests instead of only checking four HTTP/2 200s. The relay that does this moved to a shared TcpRelay test helper. - Assert JMeter's own HTTP/2 settings and the property overrides they exist for, instead of re-asserting the H2Config defaults of httpcore5. - Close the clients both suites create, shorten the re-validation wait from 2.5 s to 0.5 s and drop a pointless sleep.
…rce keys - Replace hardcoded "HTTP/2 Negotiate" string in HttpVersionComboBox with `http_version_2_negotiate` resource string - Add `http_version_2_negotiate` to `messages.properties` - Sort `http_version` property alphabetically across all 12 message bundles - Add unit test in `TestHttpVersionComboBox` verifying HTTP/2 label rendering
…ombos in clearGui - Normalize blank `httpVersion` and `httpImplementation` selections to `null` in `HttpDefaultsGui` and `HttpTestSampleGui` - Reset implementation and version combo boxes in `clearGui()` for both HTTP GUI components - Add unit tests in `TestHttpDefaultsGui` and `TestHttpTestSampleGui`
Remove HTTPConstants.HTTP_2, which duplicated HTTP_VERSION_2. Derive HTTPSamplerBase.HTTP_VERSION from the schema and rename HTTPHCAbstractImpl.HTTP_VERSION to DEFAULT_HTTP_VERSION, so the same simple name no longer means both a property name and its value. HTTPHC5Impl compared "HTTP/2" exactly but "HTTP/2 Strict" ignoring case, so a stored "http/2" fell back to HTTP/1.1 and disagreed with HTTPJavaImpl. Compare both ignoring case and cover it with a test.
The httpclient.version property now selects HTTP/1.1, HTTP/2 or HTTP/2 Strict, but it historically only took 1.0 and 1.1. Two readers were left behind: AjpSampler compared against the old spellings, so the documented values were wrong for AJP, and HTTPHCAbstractImpl defaulted to "1.1", which is not a recognized version for the HttpClient5 and Java implementations. Read the property through HTTPAbstractImpl.readDefaultHttpVersion(), which maps the legacy values 1.0 and 1.1 to HTTP/1.1, and let AjpSampler accept both 1.0 and HTTP/1.0 for its request line. Document both spellings and the AJP behaviour in jmeter.properties, properties_reference.xml and changes.xml.
changes.xml described only the HTTP/2 details and assumed the HttpClient5 implementation already existed, so the headline features of this release were missing: the new sampler implementation itself and the per-sampler HTTP Version field backed by the new HTTPSampler.httpVersion property. Add a changes.xml entry for each, and document HTTP Version in the HTTP Request Defaults properties block of component_reference.xml, which HttpDefaultsGui gained but the reference never listed. Also spell the implementations HttpClient4 and HttpClient5 as the aliases do, instead of the pre-existing HTTPClient4/HTTPClient5 typo, in component_reference.xml and in the properties_reference.xml section title.
… sampler implementations HTTPHC5Impl carried its own copy of the HttpClient4 cookie, header and form entity handling, and CountingOutputStream existed a fourth time. Lift the type-independent parts into HTTPHCAbstractImpl behind a HeaderIterable seam, add HTTPMessageSizes for the wire lengths shared with HTTPJavaImpl, and reuse jorphan's CountingOutputStream. Along the way the copies that had drifted are unified: HttpClient5 now strips a default port from a Host header of the Header Manager, multiple Cookie headers are joined, sent bytes are measured with ISO-8859-1, and jorphan's CountingOutputStream counts single byte writes again.
Replace the per implementation saveDetails/inCache/setHeaders overloads with saveDetails(ResponseHeaderSource, HTTPSampleResult), inCache(URL, RequestHeaderSource) and setHeaders(URL, RequestHeaderSink), so the Vary and cache-control handling lives in one place and the config element no longer compiles against HttpClient 5. The sampler implementations adapt their requests and responses at the call sites, and the HttpClient 4 typed methods stay as deprecated delegators.
The Java implementation shared one HttpClient with all threads, so a test drove N virtual users over a single connection and lost connection level effects. The clients are now held in an InheritableThreadLocal, so a thread still multiplexes its own requests and the parallel downloads of its embedded resources over one connection per origin, while each thread connects on its own. The former behaviour is available with the new http.java.h2.share_connections_between_threads property, which replaces http.java.h2.multiplexing and defaults to false.
…efore closing clients
The threads that download embedded resources in parallel are pooled and shared by all JMeter threads, so an InheritableThreadLocal bound them to whichever JMeter thread created them. They kept using that thread's HTTP client, which failed samples of other threads with IOException, "Socket closed" or "Connection pool shut down" as soon as the owning thread finished or started a new iteration. Look the clients up by the JMeterContext the downloader thread adopts from the JMeter thread it works for, in the HttpClient4, HttpClient5 and Java sampler implementations. This keeps HTTP/2 multiplexing between a sample and its embedded resources and stops a thread from closing clients another one is still using.
JMeter hands out an SSLContext per thread, so a pooled thread downloading embedded resources considered the HTTP/2 client of the JMeter thread it works for outdated, replaced it and shut it down while the sample and its sibling downloads were still using it, failing them with "java.io.IOException: shutdownNow".
The Java sampler implementation is left as it was, so HTTPJavaImpl keeps using HttpURLConnection and the HTTP Version combo box offers HTTP/1.1 only for it, as for HttpClient4. HTTP/2 stays with the HttpClient5 implementation. Drop the helpers only that extension needed (ConnectTimeTracker, CountingOutputHttpURLConnection, Http2CapturingHttpURLConnection, the HTTPAbstractImpl testEnded hook), the http.java.h2.* properties and the documentation of both. The shared parts of the earlier bugfixes stay: HTTPMessageSizes, SpillOutputStream, the HTTP client neutral CacheManager API and the jorphan CountingOutputStream fix.
|
Thanks for the review @vlsi. All findings have been addressed. Concerning D3, the changes in HTTPJavaImpl have been reverted. The additional HTTP/2 implementation based on java.net.http is therefore no longer part of this PR. |
3 participants
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: