mod_cache: Add a discrete commit_entity() provider function within the

mod_cache provider interface which is called to indicate to the
provider that caching is complete, giving the provider the opportunity
to commit temporary files permanently to the cache in an atomic
fashion. Move all "rename" functionality of temporary files to permanent
files within mod_disk_cache from ad hoc locations in the code to the
commit_entity() function. Instead of reusing the same variables for
temporary file handling in mod_disk_cache, introduce separate discrete
structures for each of the three cache file types, the headers file,
vary file and data file, so that the atomic rename of all three file
types within commit_entity() becomes possible. Replace the inconsistent
use of error cleanups with a formal set of pool cleanups attached to
a subpool, which is destroyed on error.

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

diff --git a/CHANGES b/CHANGES
index e303fe6ec6702d93eb43390e7812c007b6f27e29..4b78432f5fc0cbeeb748d2d8e53e1448dd97ed32 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,20 @@
 
 Changes with Apache 2.3.9
 
+  *) mod_cache: Add a discrete commit_entity() provider function within the
+     mod_cache provider interface which is called to indicate to the
+     provider that caching is complete, giving the provider the opportunity
+     to commit temporary files permanently to the cache in an atomic
+     fashion. Move all "rename" functionality of temporary files to permanent
+     files within mod_disk_cache from ad hoc locations in the code to the
+     commit_entity() function. Instead of reusing the same variables for
+     temporary file handling in mod_disk_cache, introduce separate discrete
+     structures for each of the three cache file types, the headers file,
+     vary file and data file, so that the atomic rename of all three file
+     types within commit_entity() becomes possible. Replace the inconsistent
+     use of error cleanups with a formal set of pool cleanups attached to
+     a subpool, which is destroyed on error. [Graham Leggett]
+
   *) 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

diff --git a/include/ap_mmn.h b/include/ap_mmn.h
index fbcd92018ad6bb93e80c010cedafe8841e7335d2..d31ca75cf935fc0f0f44b04a243f21993e063a41 100644 (file)
--- a/include/ap_mmn.h
+++ b/include/ap_mmn.h
@@ -251,12 +251,14 @@
  * 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.
+ * 20100916.0 (2.3.9-dev)  Add commit_entity() to the mod_cache provider
+ *                         interface.
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
 
 #ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20100912
+#define MODULE_MAGIC_NUMBER_MAJOR 20100916
 #endif
 #define MODULE_MAGIC_NUMBER_MINOR 0                     /* 0...n */
 

diff --git a/modules/cache/mod_cache.c b/modules/cache/mod_cache.c
index 71a31ea499f83dbd16d8d939421e5fe6b4975777..753afba81f19ef75c22280e630a03b8edb74fbd4 100644 (file)
--- a/modules/cache/mod_cache.c
+++ b/modules/cache/mod_cache.c
@@ -571,12 +571,14 @@ static int cache_out_filter(ap_filter_t *f, apr_bucket_brigade *bb)
  * 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.
+ * completely empty. As soon as the out brigade contains eos, call
+ * commit_entity() to finalise the cached element.
  */
 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;
+    apr_bucket *e;
 
     /* pass the brigade in into the cache provider, which is then
      * expected to move cached buckets to the out brigade, for us
@@ -601,6 +603,17 @@ static int cache_save_store(ap_filter_t *f, apr_bucket_brigade *in,
 
         }
 
+        /* does the out brigade contain eos? if so, we're done, commit! */
+        for (e = APR_BRIGADE_FIRST(cache->out);
+             e != APR_BRIGADE_SENTINEL(cache->out);
+             e = APR_BUCKET_NEXT(e))
+        {
+            if (APR_BUCKET_IS_EOS(e)) {
+                rv = cache->provider->commit_entity(cache->handle, f->r);
+                break;
+            }
+        }
+
         /* 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);
@@ -1154,6 +1167,13 @@ static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
         apr_bucket *bkt;
         int status;
 
+        /* We're just saving response headers, so we are done. Commit
+         * the response at this point, unless there was a previous error.
+         */
+        if (rv == APR_SUCCESS) {
+            rv = cache->provider->commit_entity(cache->handle, r);
+        }
+
         bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
 
         /* Restore the original request headers and see if we need to

diff --git a/modules/cache/mod_cache.h b/modules/cache/mod_cache.h
index 1d695021b025b2de493dba555cbb30dffa3b00e1..e46f2be55d4537ed0e283d4401cbb0925d9d15b4 100644 (file)
--- a/modules/cache/mod_cache.h
+++ b/modules/cache/mod_cache.h
@@ -240,6 +240,7 @@ typedef struct {
     int (*open_entity) (cache_handle_t *h, request_rec *r,
                            const char *urlkey);
     int (*remove_url) (cache_handle_t *h, apr_pool_t *p);
+    apr_status_t (*commit_entity)(cache_handle_t *h, request_rec *r);
 } cache_provider;
 
 /* A linked-list of authn providers. */

diff --git a/modules/cache/mod_disk_cache.c b/modules/cache/mod_disk_cache.c
index 2cead23bb68af31cd7f79500a3b98097359a03a8..f902c0f95d9f68ac93b8378ad6f39a9f7d4fe3fc 100644 (file)
--- a/modules/cache/mod_disk_cache.c
+++ b/modules/cache/mod_disk_cache.c
@@ -154,49 +154,57 @@ static apr_status_t safe_file_rename(disk_cache_conf *conf,
     return rv;
 }
 
-static apr_status_t file_cache_el_final(disk_cache_object_t *dobj,
+static apr_status_t file_cache_el_final(disk_cache_conf *conf, disk_cache_file_t *file,
                                         request_rec *r)
 {
-    /* move the data over */
-    if (dobj->tfd) {
-        apr_status_t rv;
+    apr_status_t rv;
+
+    /* This assumes that the tempfiles are on the same file system
+     * as the cache_root. If not, then we need a file copy/move
+     * rather than a rename.
+     */
 
-        apr_file_close(dobj->tfd);
+    /* move the file over */
+    if (file->tempfd) {
 
-        /* This assumes that the tempfile is on the same file system
-         * as the cache_root. If not, then we need a file copy/move
-         * rather than a rename.
-         */
-        rv = apr_file_rename(dobj->tempfile, dobj->datafile, r->pool);
+        rv = safe_file_rename(conf, file->tempfile, file->file, file->pool);
         if (rv != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
-                         "disk_cache: rename tempfile to datafile failed:"
-                         " %s -> %s", dobj->tempfile, dobj->datafile);
-            apr_file_remove(dobj->tempfile, r->pool);
+                         "disk_cache: rename tempfile to file failed:"
+                         " %s -> %s", file->tempfile, file->file);
+            apr_file_remove(file->tempfile, file->pool);
         }
 
-        dobj->tfd = NULL;
+        file->tempfd = NULL;
     }
 
-    return APR_SUCCESS;
+    return rv;
 }
 
-static apr_status_t file_cache_errorcleanup(disk_cache_object_t *dobj, request_rec *r)
-{
-    /* Remove the header file and the body file. */
-    apr_file_remove(dobj->hdrsfile, r->pool);
-    apr_file_remove(dobj->datafile, r->pool);
-
-    /* If we opened the temporary data file, close and remove it. */
-    if (dobj->tfd) {
-        apr_file_close(dobj->tfd);
-        apr_file_remove(dobj->tempfile, r->pool);
-        dobj->tfd = NULL;
+static apr_status_t file_cache_temp_cleanup(void *dummy) {
+    disk_cache_file_t *file = (disk_cache_file_t *)dummy;
+
+    /* clean up the temporary file */
+    if (file->tempfd) {
+        apr_file_remove(file->tempfile, file->pool);
+        file->tempfd = NULL;
     }
+    file->tempfile = NULL;
+    file->pool = NULL;
 
     return APR_SUCCESS;
 }
 
+static apr_status_t file_cache_create(disk_cache_conf *conf, disk_cache_file_t *file,
+                                      apr_pool_t *pool)
+{
+    file->pool = pool;
+    file->tempfile = apr_pstrcat(pool, conf->cache_root, AP_TEMPFILE, NULL);
+
+    apr_pool_cleanup_register(pool, file, file_cache_temp_cleanup, file_cache_temp_cleanup);
+
+    return APR_SUCCESS;
+}
 
 /* These two functions get and put state information into the data
  * file for an ap_cache_el, this state information will be read
@@ -329,6 +337,7 @@ static int create_entity(cache_handle_t *h, request_rec *r, const char *key, apr
                                                  &disk_cache_module);
     cache_object_t *obj;
     disk_cache_object_t *dobj;
+    apr_pool_t *pool;
 
     if (conf->cache_root == NULL) {
         return DECLINED;
@@ -369,9 +378,16 @@ static int create_entity(cache_handle_t *h, request_rec *r, const char *key, apr
     /* Save the cache root */
     dobj->root = apr_pstrndup(r->pool, conf->cache_root, conf->cache_root_len);
     dobj->root_len = conf->cache_root_len;
-    dobj->datafile = data_file(r->pool, conf, dobj, key);
-    dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
-    dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
+
+    apr_pool_create(&pool, r->pool);
+
+    file_cache_create(conf, &dobj->hdrs, pool);
+    file_cache_create(conf, &dobj->vary, pool);
+    file_cache_create(conf, &dobj->data, pool);
+
+    dobj->data.file = data_file(r->pool, conf, dobj, key);
+    dobj->hdrs.file = header_file(r->pool, conf, dobj, key);
+    dobj->vary.file = header_file(r->pool, conf, dobj, key);
 
     return OK;
 }
@@ -394,6 +410,7 @@ static int open_entity(cache_handle_t *h, request_rec *r, const char *key)
     cache_info *info;
     disk_cache_object_t *dobj;
     int flags;
+    apr_pool_t *pool;
 
     h->cache_obj = NULL;
 
@@ -420,42 +437,43 @@ static int open_entity(cache_handle_t *h, request_rec *r, const char *key)
     dobj->root = apr_pstrndup(r->pool, conf->cache_root, conf->cache_root_len);
     dobj->root_len = conf->cache_root_len;
 
-    dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
+    dobj->vary.file = header_file(r->pool, conf, dobj, key);
     flags = APR_READ|APR_BINARY|APR_BUFFERED;
-    rc = apr_file_open(&dobj->hfd, dobj->hdrsfile, flags, 0, r->pool);
+    rc = apr_file_open(&dobj->vary.fd, dobj->vary.file, flags, 0, r->pool);
     if (rc != APR_SUCCESS) {
         return DECLINED;
     }
 
     /* read the format from the cache file */
     len = sizeof(format);
-    apr_file_read_full(dobj->hfd, &format, len, &len);
+    apr_file_read_full(dobj->vary.fd, &format, len, &len);
 
     if (format == VARY_FORMAT_VERSION) {
         apr_array_header_t* varray;
         apr_time_t expire;
 
         len = sizeof(expire);
-        apr_file_read_full(dobj->hfd, &expire, len, &len);
+        apr_file_read_full(dobj->vary.fd, &expire, len, &len);
 
         varray = apr_array_make(r->pool, 5, sizeof(char*));
-        rc = read_array(r, varray, dobj->hfd);
+        rc = read_array(r, varray, dobj->vary.fd);
         if (rc != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
                          "disk_cache: Cannot parse vary header file: %s",
-                         dobj->hdrsfile);
+                         dobj->vary.file);
+            apr_file_close(dobj->vary.fd);
             return DECLINED;
         }
-        apr_file_close(dobj->hfd);
+        apr_file_close(dobj->vary.fd);
 
         nkey = regen_key(r->pool, r->headers_in, varray, key);
 
         dobj->hashfile = NULL;
-        dobj->prefix = dobj->hdrsfile;
-        dobj->hdrsfile = header_file(r->pool, conf, dobj, nkey);
+        dobj->prefix = dobj->vary.file;
+        dobj->hdrs.file = header_file(r->pool, conf, dobj, nkey);
 
         flags = APR_READ|APR_BINARY|APR_BUFFERED;
-        rc = apr_file_open(&dobj->hfd, dobj->hdrsfile, flags, 0, r->pool);
+        rc = apr_file_open(&dobj->hdrs.fd, dobj->hdrs.file, flags, 0, r->pool);
         if (rc != APR_SUCCESS) {
             return DECLINED;
         }
@@ -463,56 +481,74 @@ static int open_entity(cache_handle_t *h, request_rec *r, const char *key)
     else if (format != DISK_FORMAT_VERSION) {
         ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
                      "disk_cache: File '%s' has a version mismatch. File had version: %d.",
-                     dobj->hdrsfile, format);
+                     dobj->vary.file, format);
+        apr_file_close(dobj->vary.fd);
         return DECLINED;
     }
     else {
         apr_off_t offset = 0;
+
+        /* oops, not vary as it turns out */
+        dobj->hdrs.fd = dobj->vary.fd;
+        dobj->vary.fd = NULL;
+        dobj->hdrs.file = dobj->vary.file;
+
         /* This wasn't a Vary Format file, so we must seek to the
          * start of the file again, so that later reads work.
          */
-        apr_file_seek(dobj->hfd, APR_SET, &offset);
+        apr_file_seek(dobj->hdrs.fd, APR_SET, &offset);
         nkey = key;
     }
 
     obj->key = nkey;
     dobj->key = nkey;
     dobj->name = key;
-    dobj->datafile = data_file(r->pool, conf, dobj, nkey);
-    dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
+
+    apr_pool_create(&pool, r->pool);
+
+    file_cache_create(conf, &dobj->hdrs, pool);
+    file_cache_create(conf, &dobj->vary, pool);
+    file_cache_create(conf, &dobj->data, pool);
+
+    dobj->data.file = data_file(r->pool, conf, dobj, nkey);
 
     /* Open the data file */
     flags = APR_READ|APR_BINARY;
 #ifdef APR_SENDFILE_ENABLED
     /* When we are in the quick handler we don't have the per-directory
-     * configuration, so this check only takes the globel setting of
+     * configuration, so this check only takes the global setting of
      * the EnableSendFile directive into account.
      */
     flags |= AP_SENDFILE_ENABLED(coreconf->enable_sendfile);
 #endif
-    rc = apr_file_open(&dobj->fd, dobj->datafile, flags, 0, r->pool);
+    rc = apr_file_open(&dobj->data.fd, dobj->data.file, flags, 0, r->pool);
     if (rc != APR_SUCCESS) {
         ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
-                 "disk_cache: Cannot open info header file %s",  dobj->datafile);
+                 "disk_cache: Cannot open data file %s",  dobj->data.file);
+        apr_file_close(dobj->hdrs.fd);
         return DECLINED;
     }
 
-    rc = apr_file_info_get(&finfo, APR_FINFO_SIZE, dobj->fd);
+    rc = apr_file_info_get(&finfo, APR_FINFO_SIZE, dobj->data.fd);
     if (rc == APR_SUCCESS) {
         dobj->file_size = finfo.size;
     }
 
     /* Read the bytes to setup the cache_info fields */
-    rc = file_cache_recall_mydata(dobj->hfd, info, dobj, r);
+    rc = file_cache_recall_mydata(dobj->hdrs.fd, info, dobj, r);
     if (rc != APR_SUCCESS) {
         ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
-                 "disk_cache: Cannot read header file %s",  dobj->hdrsfile);
+                 "disk_cache: Cannot read header file %s",  dobj->hdrs.file);
+        apr_file_close(dobj->hdrs.fd);
         return DECLINED;
     }
 
     /* Initialize the cache_handle callback functions */
     ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
                  "disk_cache: Recalled cached URL info header %s",  dobj->name);
+
+    apr_file_close(dobj->hdrs.fd);
+
     return OK;
 }
 
@@ -535,35 +571,35 @@ static int remove_url(cache_handle_t *h, apr_pool_t *p)
     }
 
     /* Delete headers file */
-    if (dobj->hdrsfile) {
+    if (dobj->hdrs.file) {
         ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
-                     "disk_cache: Deleting %s from cache.", dobj->hdrsfile);
+                     "disk_cache: Deleting %s from cache.", dobj->hdrs.file);
 
-        rc = apr_file_remove(dobj->hdrsfile, p);
+        rc = apr_file_remove(dobj->hdrs.file, p);
         if ((rc != APR_SUCCESS) && !APR_STATUS_IS_ENOENT(rc)) {
             /* Will only result in an output if httpd is started with -e debug.
              * For reason see log_error_core for the case s == NULL.
              */
             ap_log_error(APLOG_MARK, APLOG_DEBUG, rc, NULL,
                    "disk_cache: Failed to delete headers file %s from cache.",
-                         dobj->hdrsfile);
+                         dobj->hdrs.file);
             return DECLINED;
         }
     }
 
-     /* Delete data file */
-    if (dobj->datafile) {
+    /* Delete data file */
+    if (dobj->data.file) {
         ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
-                     "disk_cache: Deleting %s from cache.", dobj->datafile);
+                     "disk_cache: Deleting %s from cache.", dobj->data.file);
 
-        rc = apr_file_remove(dobj->datafile, p);
+        rc = apr_file_remove(dobj->data.file, p);
         if ((rc != APR_SUCCESS) && !APR_STATUS_IS_ENOENT(rc)) {
             /* Will only result in an output if httpd is started with -e debug.
              * For reason see log_error_core for the case s == NULL.
              */
             ap_log_error(APLOG_MARK, APLOG_DEBUG, rc, NULL,
                       "disk_cache: Failed to delete data file %s from cache.",
-                         dobj->datafile);
+                         dobj->data.file);
             return DECLINED;
         }
     }
@@ -572,7 +608,7 @@ static int remove_url(cache_handle_t *h, apr_pool_t *p)
     if (dobj->root) {
         const char *str_to_copy;
 
-        str_to_copy = dobj->hdrsfile ? dobj->hdrsfile : dobj->datafile;
+        str_to_copy = dobj->hdrs.file ? dobj->hdrs.file : dobj->data.file;
         if (str_to_copy) {
             char *dir, *slash, *q;
 
@@ -770,7 +806,7 @@ static apr_status_t recall_headers(cache_handle_t *h, request_rec *r)
     disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
 
     /* This case should not happen... */
-    if (!dobj->hfd) {
+    if (!dobj->hdrs.fd) {
         ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
                  "disk_cache: recalling headers; but no header fd for %s", dobj->name);
         return APR_NOTFOUND;
@@ -780,10 +816,10 @@ static apr_status_t recall_headers(cache_handle_t *h, request_rec *r)
     h->resp_hdrs = apr_table_make(r->pool, 20);
 
     /* Call routine to read the header lines/status line */
-    read_table(h, r, h->resp_hdrs, dobj->hfd);
-    read_table(h, r, h->req_hdrs, dobj->hfd);
+    read_table(h, r, h->resp_hdrs, dobj->hdrs.fd);
+    read_table(h, r, h->req_hdrs, dobj->hdrs.fd);
 
-    apr_file_close(dobj->hfd);
+    apr_file_close(dobj->hdrs.fd);
 
     ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
                  "disk_cache: Recalled headers for URL %s",  dobj->name);
@@ -795,7 +831,7 @@ static apr_status_t recall_body(cache_handle_t *h, apr_pool_t *p, apr_bucket_bri
     apr_bucket *e;
     disk_cache_object_t *dobj = (disk_cache_object_t*) h->cache_obj->vobj;
 
-    apr_brigade_insert_file(bb, dobj->fd, 0, dobj->file_size, p);
+    apr_brigade_insert_file(bb, dobj->data.fd, 0, dobj->file_size, p);
 
     e = apr_bucket_eos_create(bb->bucket_alloc);
     APR_BRIGADE_INSERT_TAIL(bb, e);
@@ -865,66 +901,53 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info
              * vary format hints in the appropriate directory.
              */
             if (dobj->prefix) {
-                dobj->hdrsfile = dobj->prefix;
+                dobj->hdrs.file = dobj->prefix;
                 dobj->prefix = NULL;
             }
 
-            rv = mkdir_structure(conf, dobj->hdrsfile, r->pool);
+            rv = mkdir_structure(conf, dobj->hdrs.file, r->pool);
 
-            rv = apr_file_mktemp(&dobj->tfd, dobj->tempfile,
+            rv = apr_file_mktemp(&dobj->vary.tempfd, dobj->vary.tempfile,
                                  APR_CREATE | APR_WRITE | APR_BINARY | APR_EXCL,
-                                 r->pool);
+                                 dobj->vary.pool);
 
             if (rv != APR_SUCCESS) {
                 ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
                     "disk_cache: could not create temp file %s",
-                    dobj->tempfile);
+                    dobj->vary.tempfile);
                 return rv;
             }
 
             amt = sizeof(format);
-            apr_file_write(dobj->tfd, &format, &amt);
+            apr_file_write(dobj->vary.tempfd, &format, &amt);
 
             amt = sizeof(info->expire);
-            apr_file_write(dobj->tfd, &info->expire, &amt);
+            apr_file_write(dobj->vary.tempfd, &info->expire, &amt);
 
             varray = apr_array_make(r->pool, 6, sizeof(char*));
             tokens_to_array(r->pool, tmp, varray);
 
-            store_array(dobj->tfd, varray);
-
-            apr_file_close(dobj->tfd);
+            store_array(dobj->vary.tempfd, varray);
 
-            dobj->tfd = NULL;
+            apr_file_close(dobj->vary.tempfd);
 
-            rv = safe_file_rename(conf, dobj->tempfile, dobj->hdrsfile,
-                                  r->pool);
-            if (rv != APR_SUCCESS) {
-                ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
-                    "disk_cache: rename tempfile to varyfile failed: %s -> %s",
-                    dobj->tempfile, dobj->hdrsfile);
-                    apr_file_remove(dobj->tempfile, r->pool);
-                return rv;
-            }
-
-            dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
             tmp = regen_key(r->pool, r->headers_in, varray, dobj->name);
-            dobj->prefix = dobj->hdrsfile;
+            dobj->prefix = dobj->hdrs.file;
             dobj->hashfile = NULL;
-            dobj->datafile = data_file(r->pool, conf, dobj, tmp);
-            dobj->hdrsfile = header_file(r->pool, conf, dobj, tmp);
+            dobj->data.file = data_file(r->pool, conf, dobj, tmp);
+            dobj->hdrs.file = header_file(r->pool, conf, dobj, tmp);
         }
     }
 
 
-    rv = apr_file_mktemp(&dobj->hfd, dobj->tempfile,
+    rv = apr_file_mktemp(&dobj->hdrs.tempfd, dobj->hdrs.tempfile,
                          APR_CREATE | APR_WRITE | APR_BINARY |
-                         APR_BUFFERED | APR_EXCL, r->pool);
+                         APR_BUFFERED | APR_EXCL, dobj->hdrs.pool);
 
     if (rv != APR_SUCCESS) {
        ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
            "disk_cache: could not create temp file %s",
-           dobj->tempfile);
+           dobj->hdrs.tempfile);
         return rv;
     }
 
@@ -943,11 +966,12 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info
     iov[1].iov_base = (void*)dobj->name;
     iov[1].iov_len = disk_info.name_len;
 
-    rv = apr_file_writev(dobj->hfd, (const struct iovec *) &iov, 2, &amt);
+    rv = apr_file_writev(dobj->hdrs.tempfd, (const struct iovec *) &iov, 2, &amt);
     if (rv != APR_SUCCESS) {
-       ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+        ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
            "disk_cache: could not write info to header file %s",
-           dobj->hdrsfile);
+           dobj->hdrs.tempfile);
+        apr_file_close(dobj->hdrs.tempfd);
         return rv;
     }
 
@@ -956,11 +980,12 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info
 
         headers_out = ap_cache_cacheable_headers_out(r);
 
-        rv = store_table(dobj->hfd, headers_out);
+        rv = store_table(dobj->hdrs.tempfd, headers_out);
         if (rv != APR_SUCCESS) {
-           ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+            ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
                "disk_cache: could not write out-headers to header file %s",
-               dobj->hdrsfile);
+               dobj->hdrs.tempfile);
+            apr_file_close(dobj->hdrs.tempfd);
             return rv;
         }
     }
@@ -972,39 +997,18 @@ static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info
 
         headers_in = ap_cache_cacheable_headers_in(r);
 
-        rv = store_table(dobj->hfd, headers_in);
+        rv = store_table(dobj->hdrs.tempfd, headers_in);
         if (rv != APR_SUCCESS) {
-           ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+            ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
                "disk_cache: could not write in-headers to header file %s",
-               dobj->hdrsfile);
+               dobj->hdrs.tempfile);
+            apr_file_close(dobj->hdrs.tempfd);
             return rv;
         }
     }
 
-    apr_file_close(dobj->hfd); /* flush and close */
-
-    /* Remove old file with the same name. If remove fails, then
-     * perhaps we need to create the directory tree where we are
-     * about to write the new headers file.
-     */
-    rv = apr_file_remove(dobj->hdrsfile, r->pool);
-    if (rv != APR_SUCCESS) {
-        rv = mkdir_structure(conf, dobj->hdrsfile, r->pool);
-    }
-
-    rv = safe_file_rename(conf, dobj->tempfile, dobj->hdrsfile, r->pool);
-    if (rv != APR_SUCCESS) {
-        ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
-                     "disk_cache: rename tempfile to hdrsfile failed: %s -> %s",
-                     dobj->tempfile, dobj->hdrsfile);
-        apr_file_remove(dobj->tempfile, r->pool);
-        return rv;
-    }
-
-    dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
+    apr_file_close(dobj->hdrs.tempfd); /* flush and close */
 
-    ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
-                 "disk_cache: Stored headers for URL %s",  dobj->name);
     return APR_SUCCESS;
 }
 
@@ -1022,10 +1026,10 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r,
     /* We write to a temp file and then atomically rename the file over
      * in file_cache_el_final().
      */
-    if (!dobj->tfd) {
-        rv = apr_file_mktemp(&dobj->tfd, dobj->tempfile,
+    if (!dobj->data.tempfd) {
+        rv = apr_file_mktemp(&dobj->data.tempfd, dobj->data.tempfile,
                              APR_CREATE | APR_WRITE | APR_BINARY |
-                             APR_BUFFERED | APR_EXCL, r->pool);
+                             APR_BUFFERED | APR_EXCL, dobj->data.pool);
         if (rv != APR_SUCCESS) {
             return rv;
         }
@@ -1092,19 +1096,19 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r,
                          "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_pool_destroy(dobj->data.pool);
             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);
+        rv = apr_file_write_full(dobj->data.tempfd, str, length, &written);
         if (rv != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
                          "disk_cache: Error when writing cache file for URL %s",
                          h->cache_obj->key);
             /* Remove the intermediate cache file and return non-APR_SUCCESS */
-            file_cache_errorcleanup(dobj, r);
+            apr_pool_destroy(dobj->data.pool);
             APR_BRIGADE_CONCAT(out, dobj->bb);
             return rv;
         }
@@ -1115,7 +1119,7 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r,
                          "(%" APR_OFF_T_FMT ">%" APR_OFF_T_FMT ")",
                          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_pool_destroy(dobj->data.pool);
             APR_BRIGADE_CONCAT(out, dobj->bb);
             return APR_EGENERAL;
         }
@@ -1145,13 +1149,15 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r,
     if (seen_eos) {
         const char *cl_header = apr_table_get(r->headers_out, "Content-Length");
 
+        apr_file_close(dobj->data.tempfd);
+
         if (r->connection->aborted || r->no_cache) {
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
                          "disk_cache: Discarding body for URL %s "
                          "because connection has been aborted.",
                          h->cache_obj->key);
             /* Remove the intermediate cache file and return non-APR_SUCCESS */
-            file_cache_errorcleanup(dobj, r);
+            apr_pool_destroy(dobj->data.pool);
             return APR_EGENERAL;
         }
         if (dobj->file_size < conf->minfs) {
@@ -1160,7 +1166,7 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r,
                          "(%" APR_OFF_T_FMT "<%" APR_OFF_T_FMT ")",
                          h->cache_obj->key, dobj->file_size, conf->minfs);
             /* Remove the intermediate cache file and return non-APR_SUCCESS */
-            file_cache_errorcleanup(dobj, r);
+            apr_pool_destroy(dobj->data.pool);
             return APR_EGENERAL;
         }
         if (cl_header) {
@@ -1170,17 +1176,47 @@ static apr_status_t store_body(cache_handle_t *h, request_rec *r,
                              "disk_cache: URL %s didn't receive complete response, not caching",
                              h->cache_obj->key);
                 /* Remove the intermediate cache file and return non-APR_SUCCESS */
-                file_cache_errorcleanup(dobj, r);
+                apr_pool_destroy(dobj->data.pool);
                 return APR_EGENERAL;
             }
         }
 
-        /* All checks were fine. Move tempfile to final destination */
-        /* Link to the perm file, and close the descriptor */
-        file_cache_el_final(dobj, r);
+        /* All checks were fine, we're good to go when the commit comes */
+    }
+
+    return APR_SUCCESS;
+}
+
+static apr_status_t commit_entity(cache_handle_t *h, request_rec *r)
+{
+    disk_cache_conf *conf = ap_get_module_config(r->server->module_config,
+                                                 &disk_cache_module);
+    disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
+    apr_status_t rv;
+
+    /* move header and data tempfiles to the final destination */
+    rv = file_cache_el_final(conf, &dobj->hdrs, r);
+    if (APR_SUCCESS == rv) {
+        rv = file_cache_el_final(conf, &dobj->vary, r);
+    }
+    if (APR_SUCCESS == rv) {
+        rv = file_cache_el_final(conf, &dobj->data, r);
+    }
+
+    /* remove the cached items completely on any failure */
+    if (APR_SUCCESS != rv) {
+        remove_url(h, r->pool);
         ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
-                     "disk_cache: Body for URL %s cached.",  dobj->name);
+                     "disk_cache: commit_entity: URL '%s' not cached due to earlier disk error.",
+                     dobj->name);
     }
+    else {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
+                     "disk_cache: commit_entity: Headers and body for URL %s cached.",
+                     dobj->name);
+    }
+
+    apr_pool_destroy(dobj->data.pool);
 
     return APR_SUCCESS;
 }
@@ -1359,6 +1395,7 @@ static const cache_provider cache_disk_provider =
     &create_entity,
     &open_entity,
     &remove_url,
+    &commit_entity
 };
 
 static void disk_cache_register_hook(apr_pool_t *p)

diff --git a/modules/cache/mod_disk_cache.h b/modules/cache/mod_disk_cache.h
index bfafff91f6baf6594f482bb62a2dc22f2c95560c..67c42df1198abf56c2263a383bd367c2e53dffd8 100644 (file)
--- a/modules/cache/mod_disk_cache.h
+++ b/modules/cache/mod_disk_cache.h
@@ -51,24 +51,29 @@ typedef struct {
     apr_time_t response_time;
 } disk_cache_info_t;
 
+typedef struct {
+    apr_pool_t *pool;
+    const char *file;
+    apr_file_t *fd;
+    char *tempfile;
+    apr_file_t *tempfd;
+} disk_cache_file_t;
+
 /*
  * disk_cache_object_t
  * Pointed to by cache_object_t::vobj
  */
 typedef struct disk_cache_object {
-    const char *root;        /* the location of the cache directory */
+    const char *root;            /* the location of the cache directory */
     apr_size_t root_len;
-    char *tempfile;    /* temp file tohold the content */
     const char *prefix;
-    const char *datafile;    /* name of file where the data will go */
-    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  */
+    disk_cache_file_t data;      /* data file structure */
+    disk_cache_file_t hdrs;      /* headers file structure */
+    disk_cache_file_t vary;      /* vary file structure */
+    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_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 */