On Mon, 2020-08-17 at 08:11 +0000, Henselin, Dirk (G-GCPS) wrote:
> 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
Hi Dirk
Please submit the proposed changes as a PR at GitHub.
Cheers
Oleg
>
> -----Ursprüngliche Nachricht-----
> Von: Oleg Kalnichevski <[email protected]>
> Gesendet: Freitag, 14. August 2020 11:17
> An: HttpClient User Discussion <[email protected]>
> 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 <[email protected]>
> > Gesendet: Mittwoch, 12. August 2020 17:53
> > An: HttpClient User Discussion <[email protected]>
> > 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: [email protected]
> > For additional commands, e-mail:
> > [email protected]
> >
> > -----------------------------------------------------------------
> > ----
> > To unsubscribe, e-mail: [email protected]
> > For additional commands, e-mail:
> > [email protected]
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
