On 17 Jan 2007, at 17:32, Niklas Edmundsson wrote:
patches! And I suppose now you agree that the extra indentation on my patch stemmed from the broken nature of the original code!Actually I don't. Either you or me have misunderstood how buckets work, since the rest of the code should syntactically be equivalent. Or I'm missing some fine detail somewhere.
Perhaps I do not understand buckets fully (and brigades), but this seems to be clear enough.
The fine detail is in the original code (sorry for repeating myself):
while (e != APR_BRIGADE_SENTINEL(bb)) {
[...]
b = e;
e = APR_BUCKET_NEXT(e);
APR_BUCKET_REMOVE(b); //all buckets get removed from the
brigade
apr_bucket_destroy(b);
[...]
}
so that once we exit the loop the brigade is empty and:
/* Drop out here if this wasn't the end */
if (!APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
return APR_SUCCESS;
}
will always return.
In my code I did a check per iteration, giving a chance to all the
buckets to be checked (except the first). So, I see two problems in
my approach: I re-write the headers too often and I might miss the
EOS when there is only one bucket.
I have now a better version (patch attached) that will please you, as
I have respected the original code layout and just corrected the way
the EOS is detected.
I have also tested your patch (httpd-2.2.4-mod_disk_cache- jumbo20070117.patch) and in my limited test it works for SSI, but does not seem to be less prone than my patch of r470455 to hammering of the back-end. It is actually a tad worse.
A test on localhost with an SSI calling this script: #!/bin/sh echo `date` >> foo.log sleep 10 echo bar with: /usr/local/apache2/bin/ab -c 10 -n 20 URLgives 13 calls to the backend with yours and 12 with mine. 18 failures out of 20 (for length) in yours, and no failures in mine. Actually, it seems that yours confuses ab, as it reported a length 2 bytes short, and not corresponding to the one in the header file.
The throughput is about the same. Giuliano
mod_disk_cache.c.r470455_20070117.patch
Description: Binary data
