Re: [squid-users] Re: not caching ecap munged body

2011-02-03 Thread Amos Jeffries
Sounds good. Looks good.  If you can post that to squid-dev today it can 
get into the merge process before next release.


We need:
 * attached file suitable for applying with "patch -p0"
 * subject line having [PATCH] then a short Changelog label
 * a brief description of the problem for commit message in the body

You may have to remove squid-users from the .cc to get the attachment 
through.


Amos
--
Please be using
  Current Stable Squid 2.7.STABLE9 or 3.1.10
  Beta testers wanted for 3.2.0.4


Re: [squid-users] Re: not caching ecap munged body

2011-02-03 Thread Jonathan Wolfe
Here's the patch - one extra line necessary to make the comparison actually 
work.  Shall I file a squid bug for ecap modules removing Content-Length with 
this patch attached?

After patching, squid properly caches an adapted response with a different 
content length from the virgin response.  With Vary: Accept-Encoding coming 
from the underlying webserver, I get N variants cached, which is expected for 
just the basic Vary support in squid 3, I believe.


diff -urp squid-3.1.10/src/adaptation/ecap/Host.cc 
squid-3.1.10-ecap-patched/src/adaptation/ecap/Host.cc
--- squid-3.1.10/src/adaptation/ecap/Host.cc2010-12-22 00:46:56.0 
-0500
+++ squid-3.1.10-ecap-patched/src/adaptation/ecap/Host.cc   2011-02-03 
13:27:41.0 -0500
@@ -25,6 +25,7 @@ Adaptation::Ecap::Host::Host()
 // this code can run only once
 
 libecap::headerReferer.assignHostId(HDR_REFERER);
+libecap::headerContentLength.assignHostId(HDR_CONTENT_LENGTH);
 
 libecap::protocolHttp.assignHostId(PROTO_HTTP);
 libecap::protocolHttps.assignHostId(PROTO_HTTPS);
diff -urp squid-3.1.10/src/adaptation/ecap/MessageRep.cc 
squid-3.1.10-ecap-patched/src/adaptation/ecap/MessageRep.cc
--- squid-3.1.10/src/adaptation/ecap/MessageRep.cc  2010-12-22 
00:46:56.0 -0500
+++ squid-3.1.10-ecap-patched/src/adaptation/ecap/MessageRep.cc 2011-02-03 
13:29:48.0 -0500
@@ -48,6 +48,9 @@ Adaptation::Ecap::HeaderRep::add(const N
 HttpHeaderEntry *e = new HttpHeaderEntry(squidId, name.image().c_str(),
 value.toString().c_str());
 theHeader.addEntry(e);
+
+if (squidId == HDR_CONTENT_LENGTH)
+theMessage.content_length = theHeader.getInt64(HDR_CONTENT_LENGTH);
 }
 
 void
@@ -58,6 +61,9 @@ Adaptation::Ecap::HeaderRep::removeAny(c
 theHeader.delByName(name.image().c_str());
 else
 theHeader.delById(squidId);
+
+if (squidId == HDR_CONTENT_LENGTH)
+theMessage.content_length = theHeader.getInt64(HDR_CONTENT_LENGTH);
 }
 
 libecap::Area


Regards,
Jonathan Wolfe

On Feb 2, 2011, at 8:44 PM, Amos Jeffries wrote:

> On Wed, 2 Feb 2011 20:09:05 -0800, Jonathan Wolfe wrote:
>> Oops, here's the diff from Alex which I forgot to include below:
>> 
>> === modified file 'src/adaptation/ecap/MessageRep.cc'
>> --- src/adaptation/ecap/MessageRep.cc 2010-05-26 03:06:02 +
>> +++ src/adaptation/ecap/MessageRep.cc 2011-01-26 21:43:36 +
>> @@ -44,24 +44,30 @@
>> void
>> Adaptation::Ecap::HeaderRep::add(const Name &name, const Value &value)
>> {
>> const http_hdr_type squidId = TranslateHeaderId(name); // HDR_OTHER
> OK
>> HttpHeaderEntry *e = new HttpHeaderEntry(squidId,
>> name.image().c_str(),
>> value.toString().c_str());
>> theHeader.addEntry(e);
>> +
>> + // XXX: this is too often and not enough, on many levels
>> + theMessage.content_length = theHeader.getInt64(HDR_CONTENT_LENGTH);
>> }
>> 
>> void
>> Adaptation::Ecap::HeaderRep::removeAny(const Name &name)
>> {
>> const http_hdr_type squidId = TranslateHeaderId(name);
>> if (squidId == HDR_OTHER)
>> theHeader.delByName(name.image().c_str());
>> else
>> theHeader.delById(squidId);
>> +
>> + // XXX: this is too often and not enough, on many levels
>> + theMessage.content_length = theHeader.getInt64(HDR_CONTENT_LENGTH);
>> }
>> 
>> libecap::Area
>> Adaptation::Ecap::HeaderRep::image() const
>> {
>> MemBuf mb;
>> mb.init();
>> 
>> 
>> This only works, I think, because when removing Content-Length we end up
>> with a -1 in the length for the whole message, instead of the old (too
>> large) content length.  (There's a
>> Adaptation::Ecap::XactionRep::setAdaptedBodySize function that's
> commented
>> out in the eCAP support, presumably because it's common to not know the
>> length of the final adapted body, like in the gzip case.)
> 
> 
> I think that patch should be:
> 
> if (squidId == HDR_CONTENT_LENGTH)
>theMessage.content_length = theHeader.getInt64(HDR_CONTENT_LENGTH);
> 
> in both places to avoid that "too often" case.
> 
>> 
>> There are a few options - assume the content-length for adapted messages
>> is just the total object_len - hdr_sz, leave it always -1 unless the
> eCAP
>> module tells you otherwise by setting the Content-Length header maybe,
> or
>> finally, store it by accumulating the size of returned chunks along the
> way
>> receiving the adapted body and set it accordingly when there's no more
>> adapted body to receive.
> 
> We almost always end up with problems caused by large bodies extending
> past any possible buffer window. So sending this headers after object
> completion is not an available choice.
> 
> AFAIK if the adaptor sends its reply with chunked encoding and no
> Content-Length header at all it would work out. Squid should handle the
> dechunking and connection termination requirements for old clients.
> 
> Amos
> 



Re: [squid-users] Re: not caching ecap munged body

2011-02-02 Thread Amos Jeffries
On Wed, 2 Feb 2011 20:09:05 -0800, Jonathan Wolfe wrote:
> Oops, here's the diff from Alex which I forgot to include below:
> 
> === modified file 'src/adaptation/ecap/MessageRep.cc'
> --- src/adaptation/ecap/MessageRep.cc 2010-05-26 03:06:02 +
> +++ src/adaptation/ecap/MessageRep.cc 2011-01-26 21:43:36 +
> @@ -44,24 +44,30 @@
>  void
>  Adaptation::Ecap::HeaderRep::add(const Name &name, const Value &value)
>  {
>  const http_hdr_type squidId = TranslateHeaderId(name); // HDR_OTHER
OK
>  HttpHeaderEntry *e = new HttpHeaderEntry(squidId,
>  name.image().c_str(),
>  value.toString().c_str());
>  theHeader.addEntry(e);
> +
> + // XXX: this is too often and not enough, on many levels
> + theMessage.content_length = theHeader.getInt64(HDR_CONTENT_LENGTH);
>  }
> 
>  void
>  Adaptation::Ecap::HeaderRep::removeAny(const Name &name)
>  {
>  const http_hdr_type squidId = TranslateHeaderId(name);
>  if (squidId == HDR_OTHER)
>  theHeader.delByName(name.image().c_str());
>  else
>  theHeader.delById(squidId);
> +
> + // XXX: this is too often and not enough, on many levels
> + theMessage.content_length = theHeader.getInt64(HDR_CONTENT_LENGTH);
>  }
> 
>  libecap::Area
>  Adaptation::Ecap::HeaderRep::image() const
>  {
>  MemBuf mb;
>  mb.init();
> 
> 
> This only works, I think, because when removing Content-Length we end up
> with a -1 in the length for the whole message, instead of the old (too
> large) content length.  (There's a
> Adaptation::Ecap::XactionRep::setAdaptedBodySize function that's
commented
> out in the eCAP support, presumably because it's common to not know the
> length of the final adapted body, like in the gzip case.)


I think that patch should be:

 if (squidId == HDR_CONTENT_LENGTH)
theMessage.content_length = theHeader.getInt64(HDR_CONTENT_LENGTH);

in both places to avoid that "too often" case.

> 
> There are a few options - assume the content-length for adapted messages
> is just the total object_len - hdr_sz, leave it always -1 unless the
eCAP
> module tells you otherwise by setting the Content-Length header maybe,
or
> finally, store it by accumulating the size of returned chunks along the
way
> receiving the adapted body and set it accordingly when there's no more
> adapted body to receive.

We almost always end up with problems caused by large bodies extending
past any possible buffer window. So sending this headers after object
completion is not an available choice.

AFAIK if the adaptor sends its reply with chunked encoding and no
Content-Length header at all it would work out. Squid should handle the
dechunking and connection termination requirements for old clients.

Amos