httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Giuliano Gavazzi <dev-apache....@humph.com>
Subject Re: Solved: mod_disk_cache and mod_include bugs and suggestions
Date Wed, 17 Jan 2007 11:15:57 GMT

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

Mime
View raw message