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 "
|