Repository navigation
Don't use zero-length ByteArrayEntities for requests without a body in ApacheHttpClient #2115
Description
I've been checking out the behaviour of Apache's HttpClient with reusing pooled connections lately, and found a little unneccessity in OpenFeign's ApacheHttpClient that makes live more difficult:
If an open HTTP connection in HttpClient's connection pool is closed from the server side (or an intermediate firewall or load balancer or whatever), HttpClient does have an automatic retry mechanism that, on certain exception types and when various preconditions are fulfilled, retry fetching a connection.
One of these preconditions is that the request must not have a stream based body, since these are generally not repeatable.
Unfortunately, OpenFeign's ApacheHttpClient adds a zero-lengh ByteArrayEntity to the request if the request does not have a body, thus rendering HttpClient in a "don't retry" state.
Adding the zero-length ByteArrayEntity is not neccessary, the request just works fine without it, and then is repeatable in certain problem cases.
Note that for POST requests or any other request with a body this won't help, since obviously then you'll have a body and thus a (not empty) ByteArrayEntity. But it will help with some of the requests.
Also note that Apache's HttpClient by now has an option to check stale conditions in the pool if they've been lingering around in idle state for a configured time. This avoids a check in cases with high enough load (then the connection won't be as quickly disconnected from the other side) and still does the check in cases where it is appropriate. You'll have to set up your own PoolingHttpClientConnectionManager object for HttpClient however, to be able to set its option setValidateAfterInactivity(...). This setting helps in cases where we do have a streamed entitty in the request, but it might not help in other cases when the connection is lost before it is validated due to the inactivity timeout.
Activity
ugrepel commentedon Jul 5, 2023
sigh... it ain't that easy... (noted just now)
If I comment out that setting of a zero-byte-entity for bodyless requests, query parameters won't be set. This is due to the RequestBuilder distinguishing via the existence of an entity whether to use an InternalRequest or an InternalEntityEclosing [sic] request. And only the second one handles query parameters...
... it would still be nice to not have a body entity of zero length...
(Apache's HttpClient's retry mechanism is different from OpenFeign's in that it doesn't retry on getting something like a 503, but just on direct connection problems. It also doesn't do an exponential backoff or similar things.)
ugrepel commentedon Jul 5, 2023
Sigh... it CAN be done. The problem with just removing the empty ByteArrayEntity for bodyless requests is that with POST and PUT requests (only with those) Apache's HttpClient modifies the request from using query parameters to using url-form-encoded body parameters. Which is much closer to spec (POST/PUT shouldn't use query but form encoded params), but which is something that Apache decided to fix without being asked to do so... at least not by me...
Nevertheless: if on POST/PUT requests with query params and no body we do set the empty ByteArrayEntity, then we're (so far) fine.
This might be a moving target, depending on the inner workings of another library, but Apache's HttpClient 4.5 is in maintenance mode, only receiving critical updates, so a change of this behaviour is not to be expected.
hutiefang76 commentedon Oct 19, 2026
I reproduced the lost-connection case with the real Apache 4 client and Feign retries disabled: the bodyless GET fails after one attempt. The POST-with-body control still makes one attempt, and the bodyless GET/POST/PUT query checks pass.
The entity is repeatable; the problem is that Apache's default retry handler treats an entity-enclosing request as non-idempotent. I’ll keep the POST/PUT query workaround and the non-idempotent request behavior, and cover the eligible bodyless methods before changing the entity handling. I’m keeping HC5 out of this patch unless a separate runtime test shows it needs the same fix.
AI-assisted investigation and implementation for @hutiefang76.