This time from the top, with three part harmony AND feeling...

Revert the read-while-caching and large-file-crash fixes for mod_disk_cache,
ready to start again.

Reverted: r450105 r450188 r462571 r462601 r462696 r467655 r467684 r468044
r468373 r468409 r470455



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@502365 13f79535-47bb-0310-9956-ffa450edef68

CHANGES

@@ -61,15 +61,6 @@ Changes with Apache 2.3.0
      make sense and leads to a division by zero.  PR 40576.
      [Xuekun Hu <xuekun.hu gmail.com>]
 
-  *) mod_cache: Pass the output filter stack through the store_body()
-     hook, giving each cache backend the ability to make a better
-     decision as to how it will allocate the tasks of writing to the
-     cache and writing to the network. Previously the write to the
-     cache task needed to be complete before the same brigade was
-     written to the network, and this caused timing and memory issues
-     on large cached files. This fix replaces the previous fix for this
-     PR below. PR39380 [Graham Leggett]
-
   *) Fix issue which could cause error messages to be written to access logs
      on Win32.  PR 40476.  [Tom Donovan <Tom.Donovan acm.org>]
 
@@ -89,28 +80,13 @@ Changes with Apache 2.3.0
 
   *) mod_proxy_fcgi: Added win32 build. [Mladen Turk]
 
-  *) mod_disk_cache: Implement read-while-caching.
-     [Niklas Edmundsson <nikke acc.umu.se>]
-
-  *) mod_disk_cache: NULL fd pointers when closing them, fix missing
-     close/flush, remove some unneccessary code duplication instead
-     of calling the right helper in replace_brigade_with_cache().
-     [Niklas Edmundsson <nikke acc.umu.se>]
-
   *) sendfile_nonblocking() takes the _brigade_ as an argument, gets 
      the first bucket from the brigade, finds it not to be a FILE
      bucket and barfs. The fix is to pass a bucket rather than a brigade.
      [Niklas Edmundsson <nikke acc.umu.se>]
 
-  *) mod_disk_cache: Do away with the write-to-file-then-move-in-place
-     mentality. [Niklas Edmundsson <nikke acc.umu.se>]
-
   *) mod_rewrite: support rewritemap by SQL query [Nick Kew]
 
-  *) mod_disk_cache: Make caching of large files possible on 32bit machines
-     by determining whether the cached file should be copied on disk rather
-     than loaded into RAM. PR39380 [Niklas Edmundsson <nikke acc.umu.se>]
-
   *) mod_proxy: Print the correct error message for erroneous configured
      ProxyPass directives. PR 40439. [serai lans-tv.com]
 

modules/cache/mod_cache.c

@@ -366,7 +366,13 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
         /* pass the brigades into the cache, then pass them
          * up the filter stack
          */
-        return cache->provider->store_body(cache->handle, f, 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: Cache provider's store_body failed!");
+            ap_remove_output_filter(f);
+        }
+        return ap_pass_brigade(f->next, in);
     }
 
     /*
@@ -823,8 +829,14 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
         return ap_pass_brigade(f->next, in);
     }
 
-    return cache->provider->store_body(cache->handle, f, 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);
+    }
 
+    return ap_pass_brigade(f->next, in);
 }
 
 /*

modules/cache/mod_cache.h

@@ -210,7 +210,7 @@ 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, ap_filter_t *f, apr_bucket_brigade *b);
+    apr_status_t (*store_body)(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b);
     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,

modules/cache/mod_disk_cache.h

@@ -22,19 +22,12 @@
  */
 
 #define VARY_FORMAT_VERSION 3
-#define DISK_FORMAT_VERSION_OLD 4
+#define DISK_FORMAT_VERSION 4
-#define DISK_FORMAT_VERSION 5
 
 #define CACHE_HEADER_SUFFIX ".header"
 #define CACHE_DATA_SUFFIX   ".data"
 #define CACHE_VDIR_SUFFIX   ".vary"
 
-#define CACHE_BUF_SIZE 65536
-
-/* How long to sleep before retrying while looping */
-#define CACHE_LOOP_SLEEP 200000
-
-
 #define AP_TEMPFILE_PREFIX "/"
 #define AP_TEMPFILE_BASE   "aptmp"
 #define AP_TEMPFILE_SUFFIX "XXXXXX"
@@ -42,10 +35,9 @@
 #define AP_TEMPFILE_NAMELEN strlen(AP_TEMPFILE_BASE AP_TEMPFILE_SUFFIX)
 #define AP_TEMPFILE AP_TEMPFILE_PREFIX AP_TEMPFILE_BASE AP_TEMPFILE_SUFFIX
 
-/* Indicates the format of the header struct stored on-disk. */
-typedef apr_uint32_t disk_cache_format_t;
-
 typedef struct {
+    /* Indicates the format of the header struct stored on-disk. */
+    apr_uint32_t format;
     /* The HTTP status code returned for this response.  */
     int status;
     /* The size of the entity name that follows. */
@@ -57,9 +49,6 @@ typedef struct {
     apr_time_t expire;
     apr_time_t request_time;
     apr_time_t response_time;
-    /* The body size forced to 64bit to not break when people go from non-LFS
-     * to LFS builds */
-    apr_int64_t file_size;
 } disk_cache_info_t;
 
 /*
@@ -75,19 +64,12 @@ typedef struct disk_cache_object {
     const char *hdrsfile;    /* name of file where the hdrs will go */
     const char *hashfile;    /* Computed hash key for this URI */
     const char *name;   /* Requested URI without vary bits - suitable for mortals. */
+    const char *key;    /* On-disk prefix; URI with Vary bits (if present) */
     apr_file_t *fd;          /* data file */
     apr_file_t *hfd;         /* headers file */
     apr_file_t *tfd;         /* temporary file for data */
     apr_off_t file_size;     /*  File size of the cached data file  */
-    apr_off_t initial_size;  /*  Initial file size reported by caller */
     disk_cache_info_t disk_info; /* Header information. */
-
-    apr_interval_time_t updtimeout; /* Cache update timeout */
-
-    int skipstore;              /* Set if we should skip storing stuff */
-    int store_body_called;      /* Number of times store_body() has executed */
-    apr_bucket_brigade *tmpbb;  /* Temporary bucket brigade. */
-    apr_status_t frv;           /* Last known status of network write */
 } disk_cache_object_t;
 
 
@@ -100,7 +82,6 @@ typedef struct disk_cache_object {
 #define DEFAULT_DIRLENGTH 2
 #define DEFAULT_MIN_FILE_SIZE 1
 #define DEFAULT_MAX_FILE_SIZE 1000000
-#define DEFAULT_UPDATE_TIMEOUT apr_time_from_sec(10)
 
 typedef struct {
     const char* cache_root;
@@ -109,26 +90,6 @@ typedef struct {
     int dirlength;               /* Length of subdirectory names */
     apr_off_t minfs;             /* minimum file size for cached files */
     apr_off_t maxfs;             /* maximum file size for cached files */
-    apr_interval_time_t updtimeout;   /* Cache update timeout */
 } disk_cache_conf;
 
-#define CACHE_ENODATA (APR_OS_START_USERERR+1)
-#define CACHE_EDECLINED (APR_OS_START_USERERR+2)
-#define CACHE_EEXIST (APR_OS_START_USERERR+3)
-
-
-typedef struct diskcache_bucket_data diskcache_bucket_data;
-struct diskcache_bucket_data {
-    /* Number of buckets using this memory */
-    apr_bucket_refcount  refcount;
-    apr_file_t  *fd;
-    /* The pool into which any needed structures should
-     *  be created while reading from this file bucket */
-    apr_pool_t *readpool;
-    /* Cache update timeout */
-    apr_interval_time_t updtimeout;
-
-};
-
-
 #endif /*MOD_DISK_CACHE_H*/

modules/cache/mod_mem_cache.c

@@ -71,7 +71,6 @@ typedef struct mem_cache_object {
     long total_refs;          /**< total number of references this entry has had */
 
     apr_uint32_t pos;   /**< the position of this entry in the cache */
-    apr_status_t frv;   /* last known status of writing to the output filter */
 
 } mem_cache_object_t;
 
@@ -102,7 +101,7 @@ static mem_cache_conf *sconf;
 /* 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, ap_filter_t *f, apr_bucket_brigade *b);
+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);
 
@@ -621,10 +620,9 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info
     return APR_SUCCESS;
 }
 
-static apr_status_t store_body(cache_handle_t *h, ap_filter_t *f, apr_bucket_brigade *b)
+static apr_status_t store_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b)
 {
     apr_status_t rv;
-    request_rec *r = f->r;
     cache_object_t *obj = h->cache_obj;
     cache_object_t *tobj = NULL;
     mem_cache_object_t *mobj = (mem_cache_object_t*) obj->vobj;
@@ -669,9 +667,7 @@ static apr_status_t store_body(cache_handle_t *h, ap_filter_t *f, apr_bucket_bri
             rv = apr_file_open(&tmpfile, name, mobj->flags,
                                APR_OS_DEFAULT, r->pool);
             if (rv != APR_SUCCESS) {
-                ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
+                return rv;
-                         "mem_cache: Failed to open file '%s' while attempting to cache the file descriptor.", name);
-                return ap_pass_brigade(f->next, b);
             }
             apr_file_inherit_unset(tmpfile);
             apr_os_file_get(&(mobj->fd), tmpfile);
@@ -680,7 +676,7 @@ static apr_status_t store_body(cache_handle_t *h, ap_filter_t *f, apr_bucket_bri
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
                          "mem_cache: Cached file: %s with key: %s", name, obj->key);
             obj->complete = 1;
-            return ap_pass_brigade(f->next, b);
+            return APR_SUCCESS;
         }
 
         /* Content not suitable for fd caching. Cache in-memory instead. */
@@ -694,12 +690,7 @@ static apr_status_t store_body(cache_handle_t *h, ap_filter_t *f, apr_bucket_bri
     if (mobj->m == NULL) {
         mobj->m = malloc(mobj->m_len);
         if (mobj->m == NULL) {
-            /* we didn't have space to cache it, fall back gracefully */
+            return APR_ENOMEM;
-            cleanup_cache_object(obj);
-            ap_remove_output_filter(f);
-            ap_log_error(APLOG_MARK, APLOG_ERR, APR_ENOMEM, r->server,
-                         "mem_cache: Could not store body - not enough memory.");
-            return ap_pass_brigade(f->next, b);
         }
         obj->count = 0;
     }
@@ -720,12 +711,7 @@ static apr_status_t store_body(cache_handle_t *h, ap_filter_t *f, apr_bucket_bri
                  * buffer */
                 mobj->m = realloc(mobj->m, obj->count);
                 if (!mobj->m) {
-                    /* we didn't have space to cache it, fall back gracefully */
+                    return APR_ENOMEM;
-                    cleanup_cache_object(obj);
-                    ap_remove_output_filter(f);
-                    ap_log_error(APLOG_MARK, APLOG_ERR, APR_ENOMEM, r->server,
-                                 "mem_cache: Could not store next bit of body - not enough memory.");
-                    return ap_pass_brigade(f->next, b);
                 }
 
                 /* Now comes the crufty part... there is no way to tell the
@@ -781,36 +767,26 @@ static apr_status_t store_body(cache_handle_t *h, ap_filter_t *f, apr_bucket_bri
         }
         rv = apr_bucket_read(e, &s, &len, eblock);
         if (rv != APR_SUCCESS) {
-            cleanup_cache_object(obj);
-            /* not being able to read the bucket is fatal,
-             * return this up the filter stack
-             */
             return rv;
         }
         if (len) {
             /* Check for buffer overflow */
-            if ((obj->count + len) > mobj->m_len) {
+           if ((obj->count + len) > mobj->m_len) {
-                /* we didn't have space to cache it, fall back gracefully */
+               return APR_ENOMEM;
-                cleanup_cache_object(obj);
+           }
-                ap_remove_output_filter(f);
+           else {
-                ap_log_error(APLOG_MARK, APLOG_ERR, APR_ENOMEM, r->server,
-                             "mem_cache: Could not store body - buffer overflow.");
-                return ap_pass_brigade(f->next, b);
-            }
-            else {
                memcpy(cur, s, len);
                cur+=len;
                obj->count+=len;
-            }
+           }
         }
         /* This should not fail, but if it does, we are in BIG trouble
          * cause we just stomped all over the heap.
          */
         AP_DEBUG_ASSERT(obj->count <= mobj->m_len);
     }
-    return ap_pass_brigade(f->next, b);
+    return APR_SUCCESS;
 }
-
 /**
  * Configuration and start-up
  */