You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2010/09/13 01:16:49 UTC

svn commit: r996395 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_disk_cache.xml include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h

Author: minfrin
Date: Sun Sep 12 23:16:49 2010
New Revision: 996395

URL: http://svn.apache.org/viewvc?rev=996395&view=rev
Log:
mod_cache: Change the signature of the store_body() provider function
within the mod_cache provider interface to support an "in" brigade
and an "out" brigade instead of just a single input brigade. This
gives a cache provider the option to consume only part of the brigade
passed to it, rather than the whole brigade as was required before.
This fixes an out of memory and a request timeout condition that would
occur when the original document was a large file. Update the
mod_disk_cache provider implementation to take into account the new API.
Introduce CacheReadSize and CacheReadTime directives to mod_disk_cache
to control the amount of data to attempt to cache before sending the
data on to the client in the "out" brigade.

Modified:
    httpd/httpd/trunk/CHANGES
    httpd/httpd/trunk/docs/manual/mod/mod_disk_cache.xml
    httpd/httpd/trunk/include/ap_mmn.h
    httpd/httpd/trunk/modules/cache/mod_cache.c
    httpd/httpd/trunk/modules/cache/mod_cache.h
    httpd/httpd/trunk/modules/cache/mod_disk_cache.c
    httpd/httpd/trunk/modules/cache/mod_disk_cache.h

Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=996395&r1=996394&r2=996395&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Sun Sep 12 23:16:49 2010
@@ -2,6 +2,18 @@
 
 Changes with Apache 2.3.9
 
+  *) mod_cache: Change the signature of the store_body() provider function
+     within the mod_cache provider interface to support an "in" brigade
+     and an "out" brigade instead of just a single input brigade. This
+     gives a cache provider the option to consume only part of the brigade
+     passed to it, rather than the whole brigade as was required before.
+     This fixes an out of memory and a request timeout condition that would
+     occur when the original document was a large file. Update the
+     mod_disk_cache provider implementation to take into account the new API.
+     Introduce CacheReadSize and CacheReadTime directives to mod_disk_cache
+     to control the amount of data to attempt to cache before sending the
+     data on to the client in the "out" brigade. [Graham Leggett]
+
   *) core: Add ErrorLogFormat to allow configuring error log format, including
      additional information that is logged once per connection or request. Add
      error log IDs for connections and request to allow correlating error log

Modified: httpd/httpd/trunk/docs/manual/mod/mod_disk_cache.xml
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/mod_disk_cache.xml?rev=996395&r1=996394&r2=996395&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/manual/mod/mod_disk_cache.xml (original)
+++ httpd/httpd/trunk/docs/manual/mod/mod_disk_cache.xml Sun Sep 12 23:16:49 2010
@@ -175,4 +175,68 @@ cache</description>
 </usage>
 </directivesynopsis>
 
+<directivesynopsis>
+<name>CacheReadSize</name>
+<description>The minimum size (in bytes) of the document to read and be cached
+  before sending the data downstream</description>
+<syntax>CacheReadSize <var>bytes</var></syntax>
+<default>CacheReadSize 0</default>
+<contextlist><context>server config</context>
+    <context>virtual host</context>
+    <context>directory</context>
+    <context>.htaccess</context>
+</contextlist>
+    
+<usage>
+    <p>The <directive>CacheReadSize</directive> directive sets the
+    minimum amount of data, in bytes, to be read from the backend before the
+    data is sent to the client. The default of zero causes all data read of
+    any size to be passed downstream to the client immediately as it arrives.
+    Setting this to a higher value causes the disk cache to buffer at least
+    this amount before sending the result to the client. This can improve
+    performance when caching content from a reverse proxy.</p>
+
+    <p>This directive only takes effect when the data is being saved to the
+    cache, as opposed to data being served from the cache.</p>
+  
+    <example>
+      CacheReadSize 102400
+    </example>
+</usage>
+</directivesynopsis>
+
+<directivesynopsis>
+<name>CacheReadTime</name>
+<description>The minimum time (in milliseconds) that should elapse while reading
+  before data is sent downstream</description>
+<syntax>CacheReadTime <var>milliseconds</var></syntax>
+<default>CacheReadTime 0</default>
+<contextlist><context>server config</context>
+  <context>virtual host</context>
+  <context>directory</context>
+  <context>.htaccess</context>
+</contextlist>
+
+<usage>
+    <p>The <directive>CacheReadTime</directive> directive sets the minimum amount
+    of elapsed time that should pass before making an attempt to send data
+    downstream to the client. During the time period, data will be buffered
+    before sending the result to the client. This can improve performance when
+    caching content from a reverse proxy.</p>
+
+    <p>The default of zero disables this option.</p>
+
+    <p>This directive only takes effect when the data is being saved to the
+    cache, as opposed to data being served from the cache. It is recommended
+    that this option be used alongside the
+    <directive module="mod_disk_cache">CacheReadSize</directive> directive to
+    ensure that the server does not buffer excessively should data arrive faster
+    than expected.</p>
+
+    <example>
+      CacheReadTime 1000
+    </example>
+</usage>
+</directivesynopsis>
+  
 </modulesynopsis>

Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=996395&r1=996394&r2=996395&view=diff
==============================================================================
--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Sun Sep 12 23:16:49 2010
@@ -249,14 +249,16 @@
  *                         format handlers. Support AP_CTIME_OPTION_COMPACT in
  *                         ap_recent_ctime_ex().
  * 20100905.1 (2.3.9-dev)  Add ap_cache_check_allowed()
+ * 20100912.0 (2.3.9-dev)  Add an additional "out" brigade parameter to the
+ *                         mod_cache store_body() provider function.
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
 
 #ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20100905
+#define MODULE_MAGIC_NUMBER_MAJOR 20100912
 #endif
-#define MODULE_MAGIC_NUMBER_MINOR 1                     /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 0                     /* 0...n */
 
 /**
  * Determine if the server's current MODULE_MAGIC_NUMBER is at least a

Modified: httpd/httpd/trunk/modules/cache/mod_cache.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.c?rev=996395&r1=996394&r2=996395&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/mod_cache.c (original)
+++ httpd/httpd/trunk/modules/cache/mod_cache.c Sun Sep 12 23:16:49 2010
@@ -94,6 +94,7 @@ static int cache_quick_handler(request_r
     /* make space for the per request config */
     cache = apr_pcalloc(r->pool, sizeof(cache_request_rec));
     cache->size = -1;
+    cache->out = apr_brigade_create(r->pool, r->connection->bucket_alloc);
 
     /* store away the per request config where the API can find it */
     apr_pool_userdata_setn(cache, MOD_CACHE_REQUEST_REC, NULL, r->pool);
@@ -353,6 +354,7 @@ static int cache_handler(request_rec *r)
     /* make space for the per request config */
     cache = apr_pcalloc(r->pool, sizeof(cache_request_rec));
     cache->size = -1;
+    cache->out = apr_brigade_create(r->pool, r->connection->bucket_alloc);
 
     /* store away the per request config where the API can find it */
     apr_pool_userdata_setn(cache, MOD_CACHE_REQUEST_REC, NULL, r->pool);
@@ -564,6 +566,77 @@ static int cache_out_filter(ap_filter_t 
 }
 
 /*
+ * Having jumped through all the hoops and decided to cache the
+ * response, call store_body() for each brigade, handling the
+ * case where the provider can't swallow the full brigade. In this
+ * case, we write the brigade we were passed out downstream, and
+ * loop around to try and cache some more until the in brigade is
+ * completely empty.
+ */
+static int cache_save_store(ap_filter_t *f, apr_bucket_brigade *in,
+        cache_server_conf *conf, cache_request_rec *cache)
+{
+    int rv = APR_SUCCESS;
+
+    /* pass the brigade in into the cache provider, which is then
+     * expected to move cached buckets to the out brigade, for us
+     * to pass up the filter stack. repeat until in is empty, or
+     * we fail.
+     */
+    while (APR_SUCCESS == rv && !APR_BRIGADE_EMPTY(in)) {
+
+        rv = cache->provider->store_body(cache->handle, f->r, in, cache->out);
+        if (rv != APR_SUCCESS) {
+            ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, f->r->server,
+                         "cache: Cache provider's store_body failed!");
+            ap_remove_output_filter(f);
+
+            /* give someone else the chance to cache the file */
+            ap_cache_remove_lock(conf, f->r, cache->handle ?
+                    (char *)cache->handle->cache_obj->key : NULL, NULL);
+
+            /* give up trying to cache, just step out the way */
+            APR_BRIGADE_PREPEND(in, cache->out);
+            return ap_pass_brigade(f->next, in);
+
+        }
+
+        /* conditionally remove the lock as soon as we see the eos bucket */
+        ap_cache_remove_lock(conf, f->r, cache->handle ?
+                (char *)cache->handle->cache_obj->key : NULL, cache->out);
+
+        if (APR_BRIGADE_EMPTY(cache->out)) {
+            if (APR_BRIGADE_EMPTY(in)) {
+                /* cache provider wants more data before passing the brigade
+                 * upstream, oblige the provider by leaving to fetch more.
+                 */
+                break;
+            }
+            else {
+                /* oops, no data out, but not all data read in either, be
+                 * safe and stand down to prevent a spin.
+                 */
+                ap_log_error(APLOG_MARK, APLOG_WARNING, rv, f->r->server,
+                        "cache: Cache provider's store_body returned an "
+                         "empty brigade, but didn't consume all of the"
+                         "input brigade, standing down to prevent a spin");
+                ap_remove_output_filter(f);
+
+                /* give someone else the chance to cache the file */
+                ap_cache_remove_lock(conf, f->r, cache->handle ?
+                        (char *)cache->handle->cache_obj->key : NULL, NULL);
+
+                return ap_pass_brigade(f->next, in);
+            }
+        }
+
+        rv = ap_pass_brigade(f->next, cache->out);
+    }
+
+    return rv;
+}
+
+/*
  * CACHE_SAVE filter
  * ---------------
  *
@@ -631,28 +704,7 @@ static int cache_save_filter(ap_filter_t
      * cached file handle?
      */
     if (cache->in_checked) {
-        /* pass the brigades into the cache, then pass them
-         * up the filter stack
-         */
-        rv = cache->provider->store_body(cache->handle, r, in);
-        if (rv != APR_SUCCESS) {
-            ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
-                         "cache: Cache provider's store_body failed!");
-            ap_remove_output_filter(f);
-
-            /* give someone else the chance to cache the file */
-            ap_cache_remove_lock(conf, r, cache->handle ?
-                    (char *)cache->handle->cache_obj->key : NULL, NULL);
-        }
-        else {
-
-            /* proactively remove the lock as soon as we see the eos bucket */
-            ap_cache_remove_lock(conf, r, cache->handle ?
-                    (char *)cache->handle->cache_obj->key : NULL, in);
-
-        }
-
-        return ap_pass_brigade(f->next, in);
+        return cache_save_store(f, in, conf, cache);
     }
 
     /*
@@ -1160,21 +1212,7 @@ static int cache_save_filter(ap_filter_t
         return ap_pass_brigade(f->next, in);
     }
 
-    rv = cache->provider->store_body(cache->handle, r, in);
-    if (rv != APR_SUCCESS) {
-        ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
-                     "cache: store_body failed");
-        ap_remove_output_filter(f);
-        ap_cache_remove_lock(conf, r, cache->handle ?
-                (char *)cache->handle->cache_obj->key : NULL, NULL);
-        return ap_pass_brigade(f->next, in);
-    }
-
-    /* proactively remove the lock as soon as we see the eos bucket */
-    ap_cache_remove_lock(conf, r, cache->handle ?
-            (char *)cache->handle->cache_obj->key : NULL, in);
-
-    return ap_pass_brigade(f->next, in);
+    return cache_save_store(f, in, conf, cache);
 }
 
 /*

Modified: httpd/httpd/trunk/modules/cache/mod_cache.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.h?rev=996395&r1=996394&r2=996395&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/mod_cache.h (original)
+++ httpd/httpd/trunk/modules/cache/mod_cache.h Sun Sep 12 23:16:49 2010
@@ -231,7 +231,8 @@ struct cache_handle {
 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);
+    apr_status_t (*store_body)(cache_handle_t *h, request_rec *r, apr_bucket_brigade *in,
+                           apr_bucket_brigade *out);
     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,
@@ -271,6 +272,7 @@ typedef struct {
                                          * request
                                          */
     apr_off_t size;                     /* the content length from the headers, or -1 */
+    apr_bucket_brigade *out;            /* brigade to reuse for upstream responses */
 } cache_request_rec;
 
 

Modified: httpd/httpd/trunk/modules/cache/mod_disk_cache.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_disk_cache.c?rev=996395&r1=996394&r2=996395&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/mod_disk_cache.c (original)
+++ httpd/httpd/trunk/modules/cache/mod_disk_cache.c Sun Sep 12 23:16:49 2010
@@ -56,7 +56,8 @@ module AP_MODULE_DECLARE_DATA disk_cache
 /* 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 store_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade *in,
+                               apr_bucket_brigade *out);
 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,
@@ -1008,13 +1009,15 @@ static apr_status_t store_headers(cache_
 }
 
 static apr_status_t store_body(cache_handle_t *h, request_rec *r,
-                               apr_bucket_brigade *bb)
+                               apr_bucket_brigade *in, apr_bucket_brigade *out)
 {
     apr_bucket *e;
-    apr_status_t rv;
+    apr_status_t rv = APR_SUCCESS;
     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,
                                                  &disk_cache_module);
+    disk_cache_dir_conf *dconf = ap_get_module_config(r->per_dir_config, &disk_cache_module);
+    int seen_eos = 0;
 
     /* We write to a temp file and then atomically rename the file over
      * in file_cache_el_final().
@@ -1028,22 +1031,65 @@ static apr_status_t store_body(cache_han
         }
         dobj->file_size = 0;
     }
+    if (!dobj->bb) {
+        dobj->bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
+    }
+    if (!dobj->offset) {
+        dobj->offset = dconf->readsize;
+    }
+    if (!dobj->timeout && dconf->readtime) {
+        dobj->timeout = apr_time_now() + dconf->readtime;
+    }
 
-    for (e = APR_BRIGADE_FIRST(bb);
-         e != APR_BRIGADE_SENTINEL(bb);
-         e = APR_BUCKET_NEXT(e))
-    {
+    if (dobj->offset) {
+        apr_brigade_partition(in, dobj->offset, &e);
+    }
+
+    while (APR_SUCCESS == rv && !APR_BRIGADE_EMPTY(in)) {
         const char *str;
         apr_size_t length, written;
+
+        e = APR_BRIGADE_FIRST(in);
+
+        /* have we seen eos yet? */
+        if (APR_BUCKET_IS_EOS(e)) {
+            seen_eos = 1;
+            APR_BUCKET_REMOVE(e);
+            APR_BRIGADE_CONCAT(out, dobj->bb);
+            APR_BRIGADE_INSERT_TAIL(out, e);
+            continue;
+        }
+
+        /* 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;
+        }
+
+        /* metadata buckets are preserved as is */
+        if (APR_BUCKET_IS_METADATA(e)) {
+            APR_BUCKET_REMOVE(e);
+            APR_BRIGADE_INSERT_TAIL(dobj->bb, 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);
         if (rv != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
                          "disk_cache: Error when reading bucket for URL %s",
                          h->cache_obj->key);
             /* Remove the intermediate cache file and return non-APR_SUCCESS */
             file_cache_errorcleanup(dobj, r);
+            APR_BRIGADE_CONCAT(out, dobj->bb);
             return rv;
         }
+
+        /* write to the cache, leave if we fail */
         rv = apr_file_write_full(dobj->tfd, str, length, &written);
         if (rv != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
@@ -1051,6 +1097,7 @@ static apr_status_t store_body(cache_han
                          h->cache_obj->key);
             /* Remove the intermediate cache file and return non-APR_SUCCESS */
             file_cache_errorcleanup(dobj, r);
+            APR_BRIGADE_CONCAT(out, dobj->bb);
             return rv;
         }
         dobj->file_size += written;
@@ -1061,14 +1108,33 @@ static apr_status_t store_body(cache_han
                          h->cache_obj->key, dobj->file_size, conf->maxfs);
             /* Remove the intermediate cache file and return non-APR_SUCCESS */
             file_cache_errorcleanup(dobj, r);
+            APR_BRIGADE_CONCAT(out, dobj->bb);
             return APR_EGENERAL;
         }
+
+        /* have we reached the limit of how much we're prepared to write in one
+         * go? If so, leave, we'll get called again. This prevents us from trying
+         * to swallow too much data at once, or taking so long to write the data
+         * the client times out.
+         */
+        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;
+        }
+
     }
 
     /* 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 (seen_eos) {
         const char *cl_header = apr_table_get(r->headers_out, "Content-Length");
 
         if (r->connection->aborted || r->no_cache) {
@@ -1111,6 +1177,29 @@ static apr_status_t store_body(cache_han
     return APR_SUCCESS;
 }
 
+static void *create_dir_config(apr_pool_t *p, char *dummy)
+{
+    disk_cache_dir_conf *dconf = apr_pcalloc(p, sizeof(disk_cache_dir_conf));
+
+    dconf->readsize = DEFAULT_READSIZE;
+    dconf->readtime = DEFAULT_READTIME;
+
+    return dconf;
+}
+
+static void *merge_dir_config(apr_pool_t *p, void *basev, void *addv) {
+    disk_cache_dir_conf *new = (disk_cache_dir_conf *) apr_pcalloc(p, sizeof(disk_cache_dir_conf));
+    disk_cache_dir_conf *add = (disk_cache_dir_conf *) addv;
+    disk_cache_dir_conf *base = (disk_cache_dir_conf *) basev;
+
+    new->readsize = (add->readsize_set == 0) ? base->readsize : add->readsize;
+    new->readsize_set = add->readsize_set || base->readsize_set;
+    new->readtime = (add->readtime_set == 0) ? base->readtime : add->readtime;
+    new->readtime_set = add->readtime_set || base->readtime_set;
+
+    return new;
+}
+
 static void *create_config(apr_pool_t *p, server_rec *s)
 {
     disk_cache_conf *conf = apr_pcalloc(p, sizeof(disk_cache_conf));
@@ -1203,6 +1292,36 @@ static const char
     return NULL;
 }
 
+static const char
+*set_cache_readsize(cmd_parms *parms, void *in_struct_ptr, const char *arg)
+{
+    disk_cache_dir_conf *dconf = (disk_cache_dir_conf *)in_struct_ptr;
+
+    if (apr_strtoff(&dconf->readsize, arg, NULL, 0) != APR_SUCCESS ||
+            dconf->readsize < 0)
+    {
+        return "CacheReadSize argument must be a non-negative integer representing the max amount of data to cache in go.";
+    }
+    dconf->readsize_set = 1;
+    return NULL;
+}
+
+static const char
+*set_cache_readtime(cmd_parms *parms, void *in_struct_ptr, const char *arg)
+{
+    disk_cache_dir_conf *dconf = (disk_cache_dir_conf *)in_struct_ptr;
+    apr_off_t milliseconds;
+
+    if (apr_strtoff(&milliseconds, arg, NULL, 0) != APR_SUCCESS ||
+            milliseconds < 0)
+    {
+        return "CacheReadTime argument must be a non-negative integer representing the max amount of time taken to cache in go.";
+    }
+    dconf->readtime = apr_time_from_msec(milliseconds);
+    dconf->readtime_set = 1;
+    return NULL;
+}
+
 static const command_rec disk_cache_cmds[] =
 {
     AP_INIT_TAKE1("CacheRoot", set_cache_root, NULL, RSRC_CONF,
@@ -1215,6 +1334,10 @@ static const command_rec disk_cache_cmds
                   "The minimum file size to cache a document"),
     AP_INIT_TAKE1("CacheMaxFileSize", set_cache_maxfs, NULL, RSRC_CONF,
                   "The maximum file size to cache a document"),
+    AP_INIT_TAKE1("CacheReadSize", set_cache_readsize, NULL, RSRC_CONF,
+                  "The maximum quantity of data to attempt to read and cache in one go"),
+    AP_INIT_TAKE1("CacheReadTime", set_cache_readtime, NULL, RSRC_CONF,
+                  "The maximum time taken to attempt to read and cache in go"),
     {NULL}
 };
 
@@ -1239,8 +1362,8 @@ static void disk_cache_register_hook(apr
 
 AP_DECLARE_MODULE(disk_cache) = {
     STANDARD20_MODULE_STUFF,
-    NULL,                       /* create per-directory config structure */
-    NULL,                       /* merge per-directory config structures */
+    create_dir_config,          /* create per-directory config structure */
+    merge_dir_config,           /* merge per-directory config structures */
     create_config,              /* create per-server config structure */
     NULL,                       /* merge per-server config structures */
     disk_cache_cmds,            /* command apr_table_t */

Modified: httpd/httpd/trunk/modules/cache/mod_disk_cache.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_disk_cache.h?rev=996395&r1=996394&r2=996395&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/mod_disk_cache.h (original)
+++ httpd/httpd/trunk/modules/cache/mod_disk_cache.h Sun Sep 12 23:16:49 2010
@@ -70,6 +70,9 @@ typedef struct disk_cache_object {
     apr_file_t *tfd;         /* temporary file for data */
     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_off_t offset;            /* Max size to set aside */
+    apr_time_t timeout;          /* Max time to set aside */
 } disk_cache_object_t;
 
 
@@ -82,6 +85,8 @@ typedef struct disk_cache_object {
 #define DEFAULT_DIRLENGTH 2
 #define DEFAULT_MIN_FILE_SIZE 1
 #define DEFAULT_MAX_FILE_SIZE 1000000
+#define DEFAULT_READSIZE 0
+#define DEFAULT_READTIME 0
 
 typedef struct {
     const char* cache_root;
@@ -92,4 +97,11 @@ typedef struct {
     apr_off_t maxfs;             /* maximum file size for cached files */
 } disk_cache_conf;
 
+typedef struct {
+    apr_off_t readsize;          /* maximum data to attempt to cache in one go */
+    apr_time_t readtime;         /* maximum time taken to cache in one go */
+    int readsize_set;
+    int readtime_set;
+} disk_cache_dir_conf;
+
 #endif /*MOD_DISK_CACHE_H*/



Re: svn commit: r996395 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_disk_cache.xml include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h

Posted by Ruediger Pluem <rp...@apache.org>.

On 09/14/2010 12:38 AM, Graham Leggett wrote:
> On 13 Sep 2010, at 8:47 AM, Ruediger Pluem wrote:
> 
>> Can't this lead to a situation where buckets that follow an EOS bucket
>> (the only ones
>> I can think of are Metabuckets) get swallowed forever by mod_disk_cache?
>> These possible Metabuckets will be swallowed and added to dobj->bb,
>> but never put
>> in the out brigade. So shouldn't we just CONCAT the remaining part of
>> in to out and
>> leave?
> 
> We do need to do this, yes, thanks for catching this. Can you verify
> r996713 and confirm that it fixes it? After seeing the eos bucket, we
> enter a simple "passthrough" mode for all remaining buckets (and any
> further buckets that might arrive) and exit.

I don't have a test case at hand, but it looks fine from inspection.
BTW: Can't we avoid the loop if dobj->done and just concat the in brigade on
the out brigade and leave?

Regards

RĂ¼diger


Re: svn commit: r996395 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_disk_cache.xml include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h

Posted by Graham Leggett <mi...@sharp.fm>.
On 13 Sep 2010, at 8:47 AM, Ruediger Pluem wrote:

> Can't this lead to a situation where buckets that follow an EOS  
> bucket (the only ones
> I can think of are Metabuckets) get swallowed forever by  
> mod_disk_cache?
> These possible Metabuckets will be swallowed and added to dobj->bb,  
> but never put
> in the out brigade. So shouldn't we just CONCAT the remaining part  
> of in to out and
> leave?

We do need to do this, yes, thanks for catching this. Can you verify  
r996713 and confirm that it fixes it? After seeing the eos bucket, we  
enter a simple "passthrough" mode for all remaining buckets (and any  
further buckets that might arrive) and exit.

Regards,
Graham
--


Re: svn commit: r996395 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_disk_cache.xml include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h

Posted by Ruediger Pluem <rp...@apache.org>.

On 09/13/2010 01:16 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Sun Sep 12 23:16:49 2010
> New Revision: 996395
> 
> URL: http://svn.apache.org/viewvc?rev=996395&view=rev
> Log:
> mod_cache: Change the signature of the store_body() provider function
> within the mod_cache provider interface to support an "in" brigade
> and an "out" brigade instead of just a single input brigade. This
> gives a cache provider the option to consume only part of the brigade
> passed to it, rather than the whole brigade as was required before.
> This fixes an out of memory and a request timeout condition that would
> occur when the original document was a large file. Update the
> mod_disk_cache provider implementation to take into account the new API.
> Introduce CacheReadSize and CacheReadTime directives to mod_disk_cache
> to control the amount of data to attempt to cache before sending the
> data on to the client in the "out" brigade.
> 
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/docs/manual/mod/mod_disk_cache.xml
>     httpd/httpd/trunk/include/ap_mmn.h
>     httpd/httpd/trunk/modules/cache/mod_cache.c
>     httpd/httpd/trunk/modules/cache/mod_cache.h
>     httpd/httpd/trunk/modules/cache/mod_disk_cache.c
>     httpd/httpd/trunk/modules/cache/mod_disk_cache.h
> 

> Modified: httpd/httpd/trunk/modules/cache/mod_disk_cache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_disk_cache.c?rev=996395&r1=996394&r2=996395&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/cache/mod_disk_cache.c (original)
> +++ httpd/httpd/trunk/modules/cache/mod_disk_cache.c Sun Sep 12 23:16:49 2010

> @@ -1028,22 +1031,65 @@ static apr_status_t store_body(cache_han
>          }
>          dobj->file_size = 0;
>      }
> +    if (!dobj->bb) {
> +        dobj->bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
> +    }
> +    if (!dobj->offset) {
> +        dobj->offset = dconf->readsize;
> +    }
> +    if (!dobj->timeout && dconf->readtime) {
> +        dobj->timeout = apr_time_now() + dconf->readtime;
> +    }
>  
> -    for (e = APR_BRIGADE_FIRST(bb);
> -         e != APR_BRIGADE_SENTINEL(bb);
> -         e = APR_BUCKET_NEXT(e))
> -    {
> +    if (dobj->offset) {
> +        apr_brigade_partition(in, dobj->offset, &e);
> +    }
> +
> +    while (APR_SUCCESS == rv && !APR_BRIGADE_EMPTY(in)) {
>          const char *str;
>          apr_size_t length, written;
> +
> +        e = APR_BRIGADE_FIRST(in);
> +
> +        /* have we seen eos yet? */
> +        if (APR_BUCKET_IS_EOS(e)) {
> +            seen_eos = 1;
> +            APR_BUCKET_REMOVE(e);
> +            APR_BRIGADE_CONCAT(out, dobj->bb);
> +            APR_BRIGADE_INSERT_TAIL(out, e);
> +            continue;

Can't this lead to a situation where buckets that follow an EOS bucket (the only ones
I can think of are Metabuckets) get swallowed forever by mod_disk_cache?
These possible Metabuckets will be swallowed and added to dobj->bb, but never put
in the out brigade. So shouldn't we just CONCAT the remaining part of in to out and
leave?

Regards

RĂ¼diger