Re: [MERGE] Support large response headers
Bundle Buggy has detected this merge request. For details, see: http://squid-cache.org/bundlebuggy//request/%3C1206895312.4944.5.camel%40HenrikLaptop%3E
Re: [MERGE] Support large response headers
On Sun, 2008-04-06 at 03:40 +0200, Henrik Nordstrom wrote: > Resubmissions as I haven't received any comments on the previous merge > request. > > This patch fixes Bug #2001 by reworking how the client_side_*.cc code > deals with response headers. Instead of parsing the headers again it > clones the already parsed header and blindly skips the unparsed headers > in the data stream. bb:comment Why do we only clone replies and not requests? Can a request have a large header? Do we already deal with that without cloning? Thank you, Alex.
Re: [MERGE] Support large response headers
On Sun, 2008-04-06 at 03:40 +0200, Henrik Nordstrom wrote: > Resubmissions as I haven't received any comments on the previous merge > request. > > This patch fixes Bug #2001 by reworking how the client_side_*.cc code > deals with response headers. Instead of parsing the headers again it > clones the already parsed header and blindly skips the unparsed headers > in the data stream. bb:comment Have you tested this with an ICAP server enabled? Thanks, Alex.
Re: [MERGE] Support large response headers
sön 2008-04-06 klockan 08:33 -0600 skrev Alex Rousskov: > Why do we only clone replies and not requests? Can a request have a > large header? Do we already deal with that without cloning? Requests is already referenced and cloned if needed only, and has been since many years back. For requests we have 1. parse 2. fixup 3. forward logics 4. optional clone and rework if using a parent 6. serialization into a text blob For replies we have (ignoring ICAP) 1. Parse, and also store the received text blob 2. Fixup 3. lots of logics to determine if the response should be cached etc 4. parse the received text blob again 5. fixup to remove hop-by-hop, add our headers etc. 6. serialization into a text blob Even on cache hits it's still this convoluted in Squid-3.. 1. Swap in the object header 2. Parse the TLV + HTTP header 3. Feed the text blob (HTTP header + initial data) to client_side_reply 4. Parse the HTTP header text blob again as in 4 above.. ... This patch replaces 4 with a clone of the already received HTTP header. Regards Henrik
Re: [MERGE] Support large response headers
sön 2008-04-06 klockan 08:50 -0600 skrev Alex Rousskov: > bb:comment > > Have you tested this with an ICAP server enabled? No. My ICAP setup is quite limited. Regards Henrik
Re: [MERGE] Support large response headers
On Sun, Apr 06, 2008, Henrik Nordstrom wrote:
> Resubmissions as I haven't received any comments on the previous merge
> request.
>
> This patch fixes Bug #2001 by reworking how the client_side_*.cc code
> deals with response headers. Instead of parsing the headers again it
> clones the already parsed header and blindly skips the unparsed headers
> in the data stream.
>
> 3.0 version submitted separately.
As discussed on IRC, I'm not sure where in 2.5/2.6 the response status/headers
are allowed to grow past 4k _AND_ be parsed.
In 2.6, clientSendMoreHeaderData():
assert(http->out.offset == 0);
rep = http->reply = clientBuildReply(http, buf, size);
if (!rep) {
/* Forward as HTTP/0.9 body with no reply */
MemBuf mb;
memBufDefInit(&mb);
memBufAppend(&mb, buf, size);
http->out.offset += body_size;
comm_write_mbuf(http->conn->fd, mb, clientWriteComplete, http);
return;
}
The above code turns a "too large" reply into a HTTP/0.9 response, which
means Squid just pipes it straight to the client without any further processing.
It doesn't do any further reply processing on the reply - it just starts
feeding data to the client. The client "happens" to figure out that there
are headers. :)
I haven't checked 2.5, and its possible that my axe removal of extra
headersEnd() calls removed some magic check that didn't allow the above code
to run until the full reply was available, -but- I don't recall any spot
in the earlier code that passed storeClientCopy() a buffer greater than 4k.
This is going to take a bit of effort to fix properly in 2.HEAD/2.7 and 3.X..
Adrian
--
- Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support -
- $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: [MERGE] Support large response headers
mån 2008-04-07 klockan 01:07 +0800 skrev Adrian Chadd: > As discussed on IRC, I'm not sure where in 2.5/2.6 the response status/headers > are allowed to grow past 4k _AND_ be parsed. >From what it seems it's not... 2.7 (and 3 with the submitted patch) behaves better there.. only having problems on cache hits, and if I am not mistaken it then falls back on cache miss instead of feeding the client the raw unparsed data... Regards Henrik
Re: [MERGE] Support large response headers
On Mon, Apr 07, 2008, Henrik Nordstrom wrote: > m??n 2008-04-07 klockan 01:07 +0800 skrev Adrian Chadd: > > > As discussed on IRC, I'm not sure where in 2.5/2.6 the response > > status/headers > > are allowed to grow past 4k _AND_ be parsed. > > >From what it seems it's not... > > 2.7 (and 3 with the submitted patch) behaves better there.. only having > problems on cache hits, and if I am not mistaken it then falls back on > cache miss instead of feeding the client the raw unparsed data... Ok. I've begun shuffling around stuff in 2.HEAD to make fixing this stuff possible. The first snippet of refactoring, which has been committed: http://www.squid-cache.org/Versions/v2/HEAD/changesets/12032.patch Shuffling the storeClientReadHeaders() stuff out of client_side.c and into store_client.c in preparation to teach all the other store clients about it: http://www.creative.net.au/diffs/20080407-squid-2.HEAD-storeclient-headers-1.diff Comments please! Adrian -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: [MERGE] Support large response headers
mån 2008-04-07 klockan 13:58 +0800 skrev Adrian Chadd: > The first snippet of refactoring, which has been committed: > > http://www.squid-cache.org/Versions/v2/HEAD/changesets/12032.patch Yes... > Shuffling the storeClientReadHeaders() stuff out of client_side.c and into > store_client.c in preparation to teach all the other store clients about it: > > http://www.creative.net.au/diffs/20080407-squid-2.HEAD-storeclient-headers-1.diff > > Comments please! Not much to say other than there where it's supposed to be. I wonder why I added the function to client_side.c and not store_client.c... but I guess there was some client struct dependency in very early versions.. Regards Henrik
Re: [MERGE] Support large response headers
On Mon, Apr 07, 2008, Henrik Nordstrom wrote: > m??n 2008-04-07 klockan 13:58 +0800 skrev Adrian Chadd: > > The first snippet of refactoring, which has been committed: > > > > http://www.squid-cache.org/Versions/v2/HEAD/changesets/12032.patch > > Yes... > > > Shuffling the storeClientReadHeaders() stuff out of client_side.c and into > > store_client.c in preparation to teach all the other store clients about it: > > > > http://www.creative.net.au/diffs/20080407-squid-2.HEAD-storeclient-headers-1.diff > > > > Comments please! > > Not much to say other than there where it's supposed to be. > > I wonder why I added the function to client_side.c and not > store_client.c... but I guess there was some client struct dependency in > very early versions.. Well, I'm happy to commit this fix to Squid-2.HEAD, and then begin reworking each of the store client users to use this for fetching reply headers, which each of them wants to do. This is the "Correct" way of doing things anyway and should be done before we sit back and think about how to "fix" things, as it makes a lot of stuff quite a bit easier to do (eg, if we choose now to remap position 0 as relevant to the reply body rather than incl. headers.) I'm trying to avoid doing the much simpler solution and ignoring the rest - ie, just having a local buffer, reading the reply from the server-side into it until we have all the headers, and then just copying the first 4k out. Thats a possible stopgap, especially for Squid-2.7, but I don't want that lingering in the codebase. Adrian -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
