Yes, I can confirm that this patch works for me, too. And now I understand your point: Buffering/keeping back the output via an additional brigade until a flush occurs isn't actually needed for the cache-filter as it could simply be passed along immediately. Seems pretty reasonable.
I'll use it in (semi-)production for the next days and keep an eye on it (but probably not needed). Thanks again for your effort & Kind regards: Florian Am Samstag, den 19.11.2011, 16:59 +0200 schrieb Graham Leggett: > On 19 Nov 2011, at 3:56 PM, f_los_ch wrote: > > > Thanks for the reply and your patch - it worked! > > > > I could not longer reproduce diffs for cached/uncached files. The log > > with dumped buckets according to my previous patch is again available > > at: http://paste2.org/p/1785342 > > > > Now, when the location is spotted, maybe I can also try to isolate the > > bug even more via further debugging for developing some production-use > > patch. In the hope that really the bug was in there and the continuous > > flushing did not masquerade the underlying issue..? > > > > But that it is working for now is a real relief. > > Thanks for confirming it works. Would it be possible to try this > alternative patch? Looking at the brigade inside h, it doesn't seem to > be doing anything useful in this case, and was a hangup from older > code. This new patch takes the intermediate brigade out completely, > and should in theory use less memory and be slightly faster: What I > need to do is find out why we haven't discovered this before. > > Index: modules/cache/mod_cache_disk.c > =================================================================== > --- modules/cache/mod_cache_disk.c (revision 1203646) > +++ modules/cache/mod_cache_disk.c (working copy) > @@ -1075,9 +1075,6 @@ > disk_cache_dir_conf *dconf = ap_get_module_config(r- > >per_dir_config, &cache_disk_module); > int seen_eos = 0; > > - if (!dobj->bb) { > - dobj->bb = apr_brigade_create(r->pool, r->connection- > >bucket_alloc); > - } > if (!dobj->offset) { > dobj->offset = dconf->readsize; > } > @@ -1107,7 +1104,6 @@ > seen_eos = 1; > dobj->done = 1; > APR_BUCKET_REMOVE(e); > - APR_BRIGADE_CONCAT(out, dobj->bb); > APR_BRIGADE_INSERT_TAIL(out, e); > break; > } > @@ -1115,7 +1111,6 @@ > /* honour flush buckets, we'll get called again */ > if (APR_BUCKET_IS_FLUSH(e)) { > APR_BUCKET_REMOVE(e); > - APR_BRIGADE_CONCAT(out, dobj->bb); > APR_BRIGADE_INSERT_TAIL(out, e); > break; > } > @@ -1123,21 +1118,20 @@ > /* metadata buckets are preserved as is */ > if (APR_BUCKET_IS_METADATA(e)) { > APR_BUCKET_REMOVE(e); > - APR_BRIGADE_INSERT_TAIL(dobj->bb, e); > + APR_BRIGADE_INSERT_TAIL(out, e); > continue; > } > > /* read the bucket, write to the cache */ > rv = apr_bucket_read(e, &str, &length, APR_BLOCK_READ); > APR_BUCKET_REMOVE(e); > - APR_BRIGADE_INSERT_TAIL(dobj->bb, e); > + APR_BRIGADE_INSERT_TAIL(out, e); > if (rv != APR_SUCCESS) { > ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, > "cache_disk: Error when reading bucket for URL > %s", > h->cache_obj->key); > /* Remove the intermediate cache file and return non- > APR_SUCCESS */ > apr_pool_destroy(dobj->data.pool); > - APR_BRIGADE_CONCAT(out, dobj->bb); > return rv; > } > > @@ -1156,7 +1150,6 @@ > APR_BUFFERED | APR_EXCL, dobj- > >data.pool); > if (rv != APR_SUCCESS) { > apr_pool_destroy(dobj->data.pool); > - APR_BRIGADE_CONCAT(out, dobj->bb); > return rv; > } > dobj->file_size = 0; > @@ -1164,7 +1157,6 @@ > dobj->data.tempfd); > if (rv != APR_SUCCESS) { > apr_pool_destroy(dobj->data.pool); > - APR_BRIGADE_CONCAT(out, dobj->bb); > return rv; > } > dobj->disk_info.device = finfo.device; > @@ -1180,7 +1172,6 @@ > h->cache_obj->key); > /* Remove the intermediate cache file and return non- > APR_SUCCESS */ > apr_pool_destroy(dobj->data.pool); > - APR_BRIGADE_CONCAT(out, dobj->bb); > return rv; > } > dobj->file_size += written; > @@ -1191,7 +1182,6 @@ > h->cache_obj->key, dobj->file_size, dconf->maxfs); > /* Remove the intermediate cache file and return non- > APR_SUCCESS */ > apr_pool_destroy(dobj->data.pool); > - APR_BRIGADE_CONCAT(out, dobj->bb); > return APR_EGENERAL; > } > > @@ -1203,12 +1193,10 @@ > dobj->offset -= length; > if (dobj->offset <= 0) { > dobj->offset = 0; > - APR_BRIGADE_CONCAT(out, dobj->bb); > break; > } > if ((dconf->readtime && apr_time_now() > dobj->timeout)) { > dobj->timeout = 0; > - APR_BRIGADE_CONCAT(out, dobj->bb); > break; > } > > Index: modules/cache/mod_cache_disk.h > =================================================================== > --- modules/cache/mod_cache_disk.h (revision 1203646) > +++ modules/cache/mod_cache_disk.h (working copy) > @@ -49,7 +49,6 @@ > const char *key; /* On-disk prefix; URI with Vary > bits (if present) */ > apr_off_t file_size; /* File size of the cached data > file */ > disk_cache_info_t disk_info; /* Header information. */ > - apr_bucket_brigade *bb; /* Set aside brigade */ > apr_table_t *headers_in; /* Input headers to save */ > apr_table_t *headers_out; /* Output headers to save */ > apr_off_t offset; /* Max size to set aside */ > > Regards, > Graham > -- >
