On 17 Jan 2007, at 10:36, Niklas Edmundsson wrote:
On Wed, 17 Jan 2007, Giuliano Gavazzi wrote:
I have a solution for the r470455 mod_disk_cache not caching SSI.
There are two points where the module seems incorrect to me,
changing those makes it work:
[...]
First, don't reindent code when not needed. That only serves to
make your patch hard to read.
point taken, but at the same time I had to move in that code. Perhaps
the
1) in store_body the condition (!APR_BUCKET_IS_EOS(APR_BRIGADE_LAST
(bb))) was incorrectly stopping the flow from ever going past (for
static and dynamic pages). I moved it, changing the condition. I
will post the patch tomorrow.
From looking at the patch I can only say "huh?". The brigade is
complete when EOS is present, and only then can you complete the
storing procedure. From a quick look at your patch I can't
see how it could change things (instead of dropping out if not EOS
you have a big if-chunk if it indeed is EOS, only adding an
indentation level).
well, it is easy to check that without my patch the "Done caching"
debug message never appears.
The explanation is simple:
ignoring the difference between data and metadata treatment:
original code:
******************
while (e != APR_BRIGADE_SENTINEL(bb)) { <<<< e is not the sentinel
[...]
b = e;
e = APR_BUCKET_NEXT(e); <<<< e can be the sentinel now
APR_BUCKET_REMOVE(b); <<<< so the last is not in the bb
apr_bucket_destroy(b);
[...]
}
/* Drop out here if this wasn't the end */
if (!APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
return APR_SUCCESS; <<< and this will always return...
}
[BLOCK] <<<< never reached
******************
patched:
******************
while (e != APR_BRIGADE_SENTINEL(bb)) {
[...]
b = e;
e = APR_BUCKET_NEXT(e); <<<< e can be the sentinel now
APR_BUCKET_REMOVE(b); <<<< so the last is not in the bb
apr_bucket_destroy(b);
[...]
// EOS, so we update the headers. GG
if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
[BLOCK] <<<<< now this has a chance...
}
}
******************
I might have missed some detail, but it's not obious from the hard-
to-read patch...
2) in store_disk_headers nothing should happen (well, it should
just return or never be called) if the dobj->initial_size < 0.
It should be called, and it should do stuff.
note that I am still calling store_headers, for it has side effects
(presumably the stuff you say it should do). But store_disk_headers
should not, unless it overwrites the headers when called again with
the correct size. But this is not the case with the original code,
and after my patch from above it would end up writing a header which
was the concatenation of the wrong and the right one...
One of the points of those patches are to solve the "thundering
herd" problem, simply described as when a frequently accessed
object is expired all accesses are served directly by your backend
until one access has completed successfully and the cache has been
able to store it. This is Bad if it causes your backend to grind to
a halt.
well, I will test with a sleep in the dynamic page and hammer it with
ab, as I am doing regularly for testing...
To avoid this, the header is always written when the cache thinks
it should cache something. Other requests will find this header,
and if the size is unknown they will wait until it's updated with
the correct size, otherwise they will do read-while-caching and
return the contents as the file is being cached.
ah, I see. So store_disk_headers should be changed to rewind when it
does rewrite the header and that part of my patch changed. I will see
to it.
Those two changes make the header cache file store the correct
resource size also for dynamic pages.
It stores the size, but doing so it breaks quite a few things.
I think it would be best if someone (Graham?) could revoke the
status of mod_disk_cache on trunk to the agreed "last good" status,
which is essentially the same as 2.2.4 if I remember correctly.
As for your problems, I would recommend staying on 2.2.4 proper and
look further into the issue of expired/last-modified headers.
no. As my bug report states, and it was what prompted this thread,
2.2.4 stores and serve two different versions of a SSI page and both
corrupted.
Giuliano
