httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: svn commit: r467655 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/mod_cache.c modules/cache/mod_cache.h
Date Wed, 25 Oct 2006 15:58:43 GMT
On Wed, Oct 25, 2006 at 01:44:48PM -0000, Graham Leggett wrote:
> Author: minfrin
> Date: Wed Oct 25 06:44:47 2006
> New Revision: 467655
> 
> URL: http://svn.apache.org/viewvc?view=rev&rev=467655
> Log:
> mod_cache: Fix an out of memory condition that occurs when the
> cache tries to save huge files (greater than RAM). Buckets bigger
> than a tuneable threshold are split into smaller buckets before
> being passed to mod_disk_cache, etc. PR 39380

Another couple of hundred lines of code and even a new config directive, 
and this still doesn't get close to actually fixing the problem! -1 
already, this code is just not getting better.  mod_disk_cache is still 
liable to eat all your RAM in that apr_bucket_read() loop, the 
apr_bucket_split() is not guaranteed to work for a morphing bucket type.

It is simple enough to fix this problem without adding all this code and 
without all the stuff in r450105 too, something like the below.

Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c	(revision 450104)
+++ modules/cache/mod_cache.c	(working copy)
@@ -342,6 +342,13 @@
         ap_set_module_config(r->request_config, &cache_module, cache);
     }
 
+    if (!cache->tmpbb) {
+        cache->tmpbb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+    }
+    else {
+        apr_brigade_cleanup(cache->tmpbb);
+    }
+
     reason = NULL;
     p = r->pool;
     /*
@@ -364,7 +371,8 @@
         /* pass the brigades into the cache, then pass them
          * up the filter stack
          */
-        rv = cache->provider->store_body(cache->handle, r, in);
+        rv = cache->provider->store_body(cache->handle, r, in,
+                                         f->next, cache->tmpbb);
         if (rv != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
                          "cache: Cache provider's store_body failed!");
@@ -827,7 +835,7 @@
         return ap_pass_brigade(f->next, in);
     }
 
-    rv = cache->provider->store_body(cache->handle, r, in);
+    rv = cache->provider->store_body(cache->handle, r, in, f->next, cache->tmpbb);
     if (rv != APR_SUCCESS) {
         ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
                      "cache: store_body failed");
Index: modules/cache/mod_cache.h
===================================================================
--- modules/cache/mod_cache.h	(revision 450104)
+++ modules/cache/mod_cache.h	(working copy)
@@ -210,7 +210,13 @@
 typedef struct {
     int (*remove_entity) (cache_handle_t *h);
     apr_status_t (*store_headers)(cache_handle_t *h, request_rec *r, cache_info *i);
-    apr_status_t (*store_body)(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b);
+    /* The store_body callback writes the contents of the bucket
+     * brigade to the cache; if necessary any buckets may be flushed
+     * up the filter chain by moving them to tmpbb and passing that
+     * brigade to the f_next filter. */
+    apr_status_t (*store_body)(cache_handle_t *h, request_rec *r, 
+                               apr_bucket_brigade *b, 
+                               ap_filter_t *f_next, apr_bucket_brigade *tmpbb);
     apr_status_t (*recall_headers) (cache_handle_t *h, request_rec *r);
     apr_status_t (*recall_body) (cache_handle_t *h, apr_pool_t *p, apr_bucket_brigade *bb);

     int (*create_entity) (cache_handle_t *h, request_rec *r,
@@ -246,6 +252,7 @@
     apr_time_t lastmod;                 /* last-modified time */
     cache_info *info;                   /* current cache info */
     ap_filter_t *remove_url_filter;     /* Enable us to remove the filter */
+    apr_bucket_brigade *tmpbb;          /* Temporary bucket brigade. */
 } cache_request_rec;
 
 
Index: modules/cache/mod_disk_cache.c
===================================================================
--- modules/cache/mod_disk_cache.c	(revision 450104)
+++ modules/cache/mod_disk_cache.c	(working copy)
@@ -56,7 +56,6 @@
 /* Forward declarations */
 static int remove_entity(cache_handle_t *h);
 static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info *i);
-static apr_status_t store_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b);
 static apr_status_t recall_headers(cache_handle_t *h, request_rec *r);
 static apr_status_t recall_body(cache_handle_t *h, apr_pool_t *p, apr_bucket_brigade *bb);
 static apr_status_t read_array(request_rec *r, apr_array_header_t* arr,
@@ -977,9 +976,10 @@
 }
 
 static apr_status_t store_body(cache_handle_t *h, request_rec *r,
-                               apr_bucket_brigade *bb)
+                               apr_bucket_brigade *bb,
+                               ap_filter_t *f_next, apr_bucket_brigade *tmpbb)
 {
-    apr_bucket *e;
+    apr_bucket *e, *next;
     apr_status_t rv;
     disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
     disk_cache_conf *conf = ap_get_module_config(r->server->module_config,
@@ -998,12 +998,16 @@
         dobj->file_size = 0;
     }
 
-    for (e = APR_BRIGADE_FIRST(bb);
-         e != APR_BRIGADE_SENTINEL(bb);
-         e = APR_BUCKET_NEXT(e))
-    {
+    e = APR_BRIGADE_FIRST(bb);
+    while (e != APR_BRIGADE_SENTINEL(bb) && !APR_BUCKET_IS_EOS(e)) {
         const char *str;
         apr_size_t length, written;
+
+        if (APR_BUCKET_IS_METADATA(e)) {
+            e = APR_BUCKET_NEXT(e);
+            continue;
+        }
+
         rv = apr_bucket_read(e, &str, &length, APR_BLOCK_READ);
         if (rv != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
@@ -1032,12 +1036,33 @@
             file_cache_errorcleanup(dobj, r);
             return APR_EGENERAL;
         }
+
+        next = APR_BUCKET_NEXT(e);
+
+        /* Move the bucket to the temp brigade and flush it up the
+         * filter chain. */
+        APR_BUCKET_REMOVE(e);
+        APR_BRIGADE_INSERT_HEAD(tmpbb, e);
+        rv = ap_pass_brigade(f_next, tmpbb);
+        if (rv) {
+            ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
+                         "disk_cache: failed to flush brigade for URL %s",
+                         h->cache_obj->key);
+            /* Remove the intermediate cache file and return non-APR_SUCCESS */
+            file_cache_errorcleanup(dobj, r);
+            return rv;
+        }
+        
+        /* Discard the bucket and move on. */
+        apr_brigade_cleanup(tmpbb);
+
+        e = next;
     }
 
     /* Was this the final bucket? If yes, close the temp file and perform
      * sanity checks.
      */
-    if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
+    if (e != APR_BRIGADE_SENTINEL(bb) && APR_BUCKET_IS_EOS(e)) {
         if (r->connection->aborted || r->no_cache) {
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
                          "disk_cache: Discarding body for URL %s "

Mime
View raw message