Hello Oleg,
I extended the TestCacheEntryUpdater by the following test, which fails in the
last line:
@Test
public void testContentLengthIsNotAddedWhenTransferEncodingIsPresent()
throws IOException {
final Header[] headers = {
new BasicHeader("Date", DateUtils.formatDate(requestDate)),
new BasicHeader("ETag", "\"etag\""),
new BasicHeader("Transfer-Encoding", "chunked")};
entry = HttpTestUtils.makeCacheEntry(headers);
response.setHeaders(new Header[]{
new BasicHeader("Last-Modified",
DateUtils.formatDate(responseDate)),
new BasicHeader("Cache-Control", "public"),
new BasicHeader("Content-Length", "0")});
final HttpCacheEntry updatedEntry = impl.updateCacheEntry(null, entry,
new Date(), new Date(), response);
final Header[] updatedHeaders = updatedEntry.getAllHeaders();
headersContain(updatedHeaders, "Transfer-Encoding", "chunked");
headersNotContain(updatedHeaders, "Content-Length", "0");
}
My proposal is to make a change in method CacheEntryUpdater::mergeHeaders. It
already retains the original Content-Encoding header and it should handle the
original Content-Length header in the same way. The change in the last loop is
sufficient to fix the test above. The change in the first loop is not necessary
to fix it, but IMHO it's consistent and probably the method
CachedHttpResponseGenerator::addMissingContentLengthHeader becomes obsolete and
may be removed.
protected Header[] mergeHeaders(final HttpCacheEntry entry, final
HttpResponse response) {
if (entryAndResponseHaveDateHeader(entry, response)
&& entryDateHeaderNewerThenResponse(entry, response)) {
// Don't merge headers, keep the entry's headers as they are newer.
return entry.getAllHeaders();
}
final HeaderGroup headerGroup = new HeaderGroup();
headerGroup.setHeaders(entry.getAllHeaders());
// Remove cache headers that match response
for (final HeaderIterator it = response.headerIterator(); it.hasNext();
) {
final Header responseHeader = it.nextHeader();
- // Since we do not expect a content in a 304 response, should
retain the original Content-Encoding header
- if (HTTP.CONTENT_ENCODING.equals(responseHeader.getName())) {
+ // Since we do not expect a content in a 304 response, should
retain the original Content-Encoding and Content-Length header
+ if (HTTP.CONTENT_ENCODING.equals(responseHeader.getName())
+ || HTTP.CONTENT_LEN.equals(responseHeader.getName())) {
continue;
}
final Header[] matchingHeaders =
headerGroup.getHeaders(responseHeader.getName());
for (final Header matchingHeader : matchingHeaders) {
headerGroup.removeHeader(matchingHeader);
}
}
// remove cache entry 1xx warnings
for (final HeaderIterator it = headerGroup.iterator(); it.hasNext(); ) {
final Header cacheHeader = it.nextHeader();
if
(HeaderConstants.WARNING.equalsIgnoreCase(cacheHeader.getName())) {
final String warningValue = cacheHeader.getValue();
if (warningValue != null && warningValue.startsWith("1")) {
it.remove();
}
}
}
for (final HeaderIterator it = response.headerIterator(); it.hasNext();
) {
final Header responseHeader = it.nextHeader();
- // Since we do not expect a content in a 304 response, should
avoid updating Content-Encoding header
- if (HTTP.CONTENT_ENCODING.equals(responseHeader.getName())) {
+ // Since we do not expect a content in a 304 response, should
avoid updating Content-Encoding and Content-Length header
+ if (HTTP.CONTENT_ENCODING.equals(responseHeader.getName())
+ || HTTP.CONTENT_LEN.equals(responseHeader.getName())) {
continue;
}
headerGroup.addHeader(responseHeader);
}
return headerGroup.getAllHeaders();
}
Best regards
Dirk
-----Ursprüngliche Nachricht-----
Von: Oleg Kalnichevski <ol...@apache.org>
Gesendet: Freitag, 14. August 2020 11:17
An: HttpClient User Discussion <httpclient-users@hc.apache.org>
Betreff: Re: AW: CachingHttpClient fails to revalidate cache entry in case of
chunked transfer encoding
On Thu, 2020-08-13 at 19:32 +0000, Henselin, Dirk (G-GCPS) wrote:
> Hello Oleg,
>
> yes, `Transfer-Encoding` and `Content-Length` headers should be
> mutually exclusive and because I use chunked transfer, the `Transfer-
> Encoding` header is set in the response while the `Content-Length`
> header is not. In case of a 304 during a revalidation, the header
> contains Content-Length=0. Probably a proxy is responsible for this,
> just like the comment "Some well-known proxies respond with Content-
> Length=0, when returning 304" in the method
> CachedHttpResponseGenerator::addMissingContentLengthHeader is saying.
> In CacheEntryUpdater::mergeHeaders the Content-Length=0 is merged into
> the cached entry, but the cached entry contains also a
> `Transfer-Encoding` header, so in the cached entry these headers
> aren't mutually exclusive anymore. Because of the `Transfer-Encoding`
> header the method
> CachedHttpResponseGenerator::addMissingContentLengthHeader isn't
> fixing the `Content-Length` header and Content-Length=0 causes
> returning null instead of the cached content. IMHO the `Content-
> Length` header should not be merged into the cached response in case
> of a 304, at least if the cached entry contains a `Transfer-Encoding`
> header.
>
> Best regards
> Dirk
>
Dirk
The original developers of the cache module are no longer with the project. I
do not know if there was a rationale for this decision.
Please either propose a patch as a PR at GitHub or produce a reproducer I could
run locally if you expect me to look into the problem.
Oleg
> -----Ursprüngliche Nachricht-----
> Von: Oleg Kalnichevski <ol...@apache.org>
> Gesendet: Mittwoch, 12. August 2020 17:53
> An: HttpClient User Discussion <httpclient-users@hc.apache.org>
> Betreff: Re: CachingHttpClient fails to revalidate cache entry in case
> of chunked transfer encoding
>
> On Wed, 2020-08-12 at 08:35 +0000, Henselin, Dirk (G-GCPS) wrote:
> > Hello,
> >
> > English is not my native language; please excuse typing errors.
> > I use HttpClient 4.5.12.
> >
> > I found that despite a cache hit, the CachingHttpClient returns
> > null, if the content-length header was missing in the original
> > response.
> > It
> > worked fine after forcing the server to add the content-length
> > header, but this isn't possible in case of chunked transfer
> > encoding.
> > I looked into the source code and found, that the content-length
> > header of a subsequent response is merged into the cache entry's
> > response headers, if the content-length header was missing in the
> > original response. In case of a 304 content-length=0 is merged into
> > the cache entry and this causes returning null instead of the cached
> > entity.
> > In CachedHttpResponseGenerator I found the following method:
> >
> > private void addMissingContentLengthHeader(final HttpResponse
> > response, final HttpEntity entity) {
> > if (transferEncodingIsPresent(response)) {
> > return;
> > }
> > // Some well known proxies respond with Content-Length=0,
> > when returning 304. For robustness, always
> > // use the cached entity's content length, as modern
> > browsers do.
> > final Header contentLength = new
> > BasicHeader(HTTP.CONTENT_LEN,
> > Long.toString(entity.getContentLength()));
> > response.setHeader(contentLength);
> > }
> >
> > Obviously, this method adds a missing content length header, but not
> > in case of chunked transfer encoding. How can I solve this?
> >
>
> Dirk,
>
> The behavior of this method looks correct to me as `Transfer-
> Encoding` and `Content-Length` headers are mutually exclusive.
>
> Could you please provide a reproducer for your original problem?
>
> Oleg
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpclient-users-unsubscr...@hc.apache.org
> For additional commands, e-mail: httpclient-users-h...@hc.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpclient-users-unsubscr...@hc.apache.org
> For additional commands, e-mail: httpclient-users-h...@hc.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-users-unsubscr...@hc.apache.org
For additional commands, e-mail: httpclient-users-h...@hc.apache.org
