[ffang]

…the HttpClient conduit

With chunking disallowed, HttpClientHTTPConduit buffers the request and publishes it with a Content-Length. For an asynchronous call the caller closes the output stream without waiting for the HttpClient, so the publisher could be closed before the HttpClient asked for the content length and subscribed, and the request went out chunked without a body.

The publisher now keeps the buffered body itself, so contentLength() and subscribe() see the complete body however late they are called.

[vp340]

Hi @ffang ,
I tested this solution against my camel reproducer and it LGTM... all my attempts were good 💯 .

Just out of curiosity, given your knowledge of the codebase, I wonder why this particular problem showed up mainly in local test with wiremock ... and it's occurring in production only to a very minor extent.
My assumption of a networks difference could be the cause for the race condition to happens more frequently only in localhost... as I mentioned in the Jira ticket?
I am a very curious person :) .
I tried debug further, but the game wasn't worth the candle and didn't have much time.

Thank,
Valentino Porta

[ffang]

Hi @ffang , I tested this solution against my camel reproducer and it LGTM... all my attempts were good 💯 .

Just out of curiosity, given your knowledge of the codebase, I wonder why this particular problem showed up mainly in local test with wiremock ... and it's occurring in production only to a very minor extent. My assumption of a networks difference could be the cause for the race condition to happens more frequently only in localhost... as I mentioned in the Jira ticket? I am a very curious person :) . I tried debug further, but the game wasn't worth the candle and didn't have much time.

Thank, Valentino Porta

Hi @vp340,

Thanks for testing the fix against your Camel reproducer, and for confirming it works. That's very helpful.

I think your network theory is right in spirit. It's a race between two threads, so anything that changes their relative timing changes how often it shows up.

With chunking disabled, CXF buffers the request and gives the HttpClient a publisher that reports the buffered size. For an asynchronous call, two things happen in parallel:

• The caller thread writes the body into the buffer and closes the stream. Closing hands the response handling to the work queue without waiting, then drops the buffer (pout = null and publisher.close()).
• The HttpClient thread gets a connection (a new one or a pooled one), asks for the content length, writes the request headers, and only then subscribes to the body.

In the old code, the body was sent only if the HttpClient finished all of that before the caller dropped the buffer. If it was too late, it got either 0/206 ("Too few bytes returned by the publisher") or a chunked request with no body. Which thread wins depends on:

• whether the connection is new or reused, and the latency to open it;
• how long the headers take to write;
• how much of the body was already buffered when the send started;
• JIT warm-up;
• thread scheduling and load on the machine.

That fits what you saw. The first call is slow on the caller side (cold JVM), so the HttpClient wins. Against a local WireMock everything after that is warm and fast, so the caller wins every time. With a remote server or a production network, the timing varies, so the outcome varies too. I haven't measured exactly why your production setup lands at 0.01%, but I wouldn't read too much into the exact numbers.

With this PR the publisher keeps its own reference to the buffered body, so the order of the two threads no longer matters. Once it's in, you shouldn't need cxf-rt-transports-http-hc or AllowChunking=true as a workaround.

Thanks,
Freeman

[vp340]

Hi @ffang ,
Thanks a lot for your accurate analysis and explanation !

With this PR the publisher keeps its own reference to the buffered body, so the order of the two threads no longer matters. Once it's in, you shouldn't need cxf-rt-transports-http-hc or AllowChunking=true as a workaround.

Sure thanks! Being the error cases in production so low we will update to the next 4.1.10 (or 4.2.5) not with a rush. I believe things have been this way for years (way before I even start working) and no one has ever complained about that ...till I discover the problem during local tests.
: /

In the old code, the body was sent only if the HttpClient finished all of that before the caller dropped the buffer. If it was too late, it got either 0/206 ("Too few bytes returned by the publisher") or a chunked request with no body. Which thread wins depends on:

• whether the connection is new or reused, and the latency to open it; (in production this should slow down the httpClient thread )
• how long the headers take to write (in production this should take more time... so slow down the httpClient thread) ;
• how much of the body was already buffered when the send started;
• JIT warm-up; (in production this should be already the hottest things running on our ocp ... so this could favor both http-nio or httpClient threads ...)
• thread scheduling and load on the machine. (this also can be non-deterministic in a production environment)

So all this things or go againt the httpClient thread (so more error) or should affect both thread . A such big difference in failure rate is quite sus, so probably the last 2 points are the ones that influence the most and decide to let the httpClient thread win (for our sake) .

Thanks a lot for all the excellent job 👍 ... if U need anything I'm here to help.
Have a great day!

Cheers,
Valentino Porta

[coheigea]

A few suggestions:

• Drop the notifyAll in subscribe(). The only code that waits on body is contentLength(). Waking it from subscribe() before the buffer is closed would make it return a too-small length. The HttpClient calls the two in sequence, so it can’t happen in practice, but the block does nothing useful and invites that misreading. It’s left over from the piped publisher, where isConnectionAttemptCompleted waits on pout.
• Harden the wait in contentLength(). It’s a single if (!closed) wait(timeout), so a spurious wakeup returns early. If the wait ends before the buffer is closed (the connection timeout elapses, e.g. a body that takes more than 30s to write), contentLength() returns a partial size. subscribe() then throws IllegalStateException from getInputStream(), because JDK InputStreamPublisher.subscribe calls the supplier immediately. The old code behaved the same, so this isn’t a regression. A while (!body.closed) loop against a deadline would fix the first case. For the second, failing clearly would beat sending a wrong Content-Length.
• publisher.stream now only exists to set connectionComplete. On the async path it’s already null when subscribe() runs, so connectionComplete is never set. Today that only matters for getHttpsURLConnectionInfo(), which spins on that flag. A short comment would stop someone from “simplifying” the field away.
• Systest nit: The test imports com.sun.net.httpserver.HttpServer, but org.apache.cxf.systest.http already has its own HttpServer class. It compiles (the import wins), but a fully-qualified name or a different variable name would read more clearly.

[ffang]

A few suggestions:

• Drop the notifyAll in subscribe(). The only code that waits on body is contentLength(). Waking it from subscribe() before the buffer is closed would make it return a too-small length. The HttpClient calls the two in sequence, so it can’t happen in practice, but the block does nothing useful and invites that misreading. It’s left over from the piped publisher, where isConnectionAttemptCompleted waits on pout.
• Harden the wait in contentLength(). It’s a single if (!closed) wait(timeout), so a spurious wakeup returns early. If the wait ends before the buffer is closed (the connection timeout elapses, e.g. a body that takes more than 30s to write), contentLength() returns a partial size. subscribe() then throws IllegalStateException from getInputStream(), because JDK InputStreamPublisher.subscribe calls the supplier immediately. The old code behaved the same, so this isn’t a regression. A while (!body.closed) loop against a deadline would fix the first case. For the second, failing clearly would beat sending a wrong Content-Length.
• publisher.stream now only exists to set connectionComplete. On the async path it’s already null when subscribe() runs, so connectionComplete is never set. Today that only matters for getHttpsURLConnectionInfo(), which spins on that flag. A short comment would stop someone from “simplifying” the field away.
• Systest nit: The test imports com.sun.net.httpserver.HttpServer, but org.apache.cxf.systest.http already has its own HttpServer class. It compiles (the import wins), but a fully-qualified name or a different variable name would read more clearly.

Thanks @coheigea for the feedback, I agree all points here and will revise accordingly.

It's done by this commit
e48a2ff

Freeman

[coheigea]

I think the last commit turns an existing stall into a hang when HTTPS, a MessageTrustDecider, disallowed chunking and an unknown content length are combined.

How it happens

1. On the first write, handleHeadersTrustCaching() sends the request (sendAsync) and then calls makeTrustDecision() → getHttpsURLConnectionInfo().
2. getHttpsURLConnectionInfo() spins on Thread.yield() until connectionComplete is true. The only place that sets it is the publisher's subscribe().
3. The HttpClient thread first calls HttpClientSizedBodyPublisher.contentLength(), which waits until the body has been completely written.
4. The caller is stuck in step 2, so it never writes the body.

Before this PR: contentLength() returned the partial length when the connection timeout expired. The HttpClient then subscribed, which set connectionComplete, so the caller got out of the loop and the request failed after about 30s.

With this PR: contentLength() throws when the timeout expires, so subscribe() never runs and connectionComplete is never set. The loop has no other way out: it doesn't check whether the request has failed. The calling thread therefore spins at full CPU forever, and the recorded failure never reaches it.

Suggested fix for this PR: let the loop in getHttpsURLConnectionInfo() also stop once future.isDone(), and rethrow the failure. This restores a clean error after the connection timeout. A test with HTTPS, a trust decider and chunking disallowed would cover it.

Possible follow-up, as a separate JIRA: also set connectionComplete in contentLength(), before it waits. For HTTP/1.1 the HttpClient only asks for the length after it has connected and completed the TLS handshake, so the trust decision could go ahead and the request would succeed. The ordering for HTTP/2 needs checking before relying on this.

[ffang]

Possible follow-up, as a separate JIRA: also set connectionComplete in contentLength(), before it waits. For HTTP/1.1 the HttpClient only asks for the length after it has connected and completed the TLS handshake, so the trust decision could go ahead and the request would succeed. The ordering for HTTP/2 needs checking before relying on this.

Thanks @coheigea, good catch. I reproduced it: with HTTPS, a trust decider and chunking disallowed, the last commit made the call spin forever, where before it failed after the connection timeout. While checking, I found that a refused connection with a trust decider spins forever too, even with chunking allowed, because subscribe() never runs, so connectionComplete is never set.

I've done both in this PR, in 6c88b674d4:

• The loop in getHttpsURLConnectionInfo() now stops once the request has failed, and rethrows the failure.
• HttpClientSizedBodyPublisher.contentLength() sets connectionComplete before it waits for the body. So the trust decision goes ahead, the body gets written, and the request succeeds instead of failing after the connection timeout.

Since the follow-up you suggested is part of this PR, a separate JIRA isn't needed. I checked the HTTP/2 ordering: the HttpClient asks for the content length only once the connection is set up, for HTTP/2 as well as HTTP/1.1, so for HTTPS the TLS handshake is already done. The new HTTP/2 test below confirms it.

New tests:

• HTTPSConduitTest.testHttpsTrustWithoutChunking: a trusted peer succeeds and an untrusted one fails, without chunking.
• HTTPSConduitTest.testHttpsTrustConnectionRefused: with a trust decider and nothing listening on the port, the call fails rather than hangs, with chunking allowed or not.
• JettyClientServerHttp2Test.testHttpsTrustWithoutChunkingHttp2: the same trusted case with HTTP/2 forced. The server echoes the protocol it received, and the test asserts HTTP/2.0.

Code	testHttpsTrustWithoutChunking (HTTP/1.1)	testHttpsTrustWithoutChunkingHttp2
Previous commit	hangs	hangs
Only the loop change	fails after the connection timeout	fails after the connection timeout
Both changes	passes	passes

testHttpsTrustConnectionRefused hangs on the previous commit and passes with the loop change.

Freeman