httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Florian S." <f_los...@yahoo.com>
Subject Re: corrupt brigades when using mod_cache in reverse proxy
Date Sat, 19 Nov 2011 19:42:04 GMT
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
> --
> 



Mime
View raw message