Sander Striker wrote:
> [EMAIL PROTECTED] wrote:
[..cut..]
>> Is this behaviour intended and compliant with the RFC?
>
>
> Not to my knowlegde. Given that mod_mem_cache and mod_disk_cache are doing
> different things is pretty much indicative that one of the two is wrong ;).
That was also my thought.
>
>> The reason for this behaviour is that the remove_url function of
>> mod_disk_cache is a dummy function
>> (BTW: mod_mem_cache seems to really remove the cache entry in
>> remove_url).
>> If this behaviour is not intended I would have a look into this to
>> create a patch.
>
>
> Please do!
>
I created a patch but the problem turned out to be more complex than I thought
originally. So a close look on the patch is definitely a good thing. Some
comments:
1. I had to adjust the cache provider API for remove_url as I need the
request_rec
struct to remove the files correctly in mod_disk_cache.
2. It turned out that 404 responses are not passed down the filter chain the
way I expected.
Adjusting the default handler again proved that the changes to
mod_disk_cache worked
(files got deleted), but this broke any error page handling in Apache. So I
tried to address
this problem at other locations of the code. I detected two cases:
1. Apache generated error messages or redirect to external source.
2. Custom local error documents.
In the first case I use the insert_error_filter hook to ensure that the
CACHE_SAVE filter
is reinserted to the filter chain if it has been inserted before during the
request.
In the second case the filter chain is run, but with the wrong URI. So I
checked if there
is a previous request (r->prev) and if it has the same status code (this
happens in a section
where we only handle uncachable status codes). If this is the case I assume
that I should delete
the URL from the previous request from the cache.
So any comments / thoughts on this?
Regards
R�diger
Index: httpd-trunk/modules/cache/mod_mem_cache.c
===================================================================
--- httpd-trunk/modules/cache/mod_mem_cache.c (revision 178625)
+++ httpd-trunk/modules/cache/mod_mem_cache.c (working copy)
@@ -601,7 +601,7 @@
/* remove_url()
* Notes:
*/
-static int remove_url(const char *key)
+static int remove_url(const char *key, request_rec *r)
{
cache_object_t *obj;
int cleanup = 0;
Index: httpd-trunk/modules/cache/mod_cache.c
===================================================================
--- httpd-trunk/modules/cache/mod_cache.c (revision 178625)
+++ httpd-trunk/modules/cache/mod_cache.c (working copy)
@@ -121,6 +121,8 @@
"Adding CACHE_SAVE filter.");
/* add cache_save filter to cache this request */
+ apr_table_set(r->notes, CACHE_SAVE_FILTER_INSERTED,
+ CACHE_SAVE_FILTER_INSERTED);
ap_add_output_filter_handle(cache_save_filter_handle, NULL, r,
r->connection);
}
@@ -254,6 +256,7 @@
if (r->no_cache ||
(!conf->store_nostore &&
ap_cache_liststr(NULL, cc_in, "no-store", NULL))) {
+ apr_table_unset(r->notes, CACHE_SAVE_FILTER_INSERTED);
ap_remove_output_filter(f);
return ap_pass_brigade(f->next, in);
}
@@ -293,6 +296,7 @@
*/
rv = cache->provider->store_body(cache->handle, r, in);
if (rv != APR_SUCCESS) {
+ apr_table_unset(r->notes, CACHE_SAVE_FILTER_INSERTED);
ap_remove_output_filter(f);
}
return ap_pass_brigade(f->next, in);
@@ -440,13 +444,31 @@
if (reason) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"cache: %s not cached. Reason: %s", url, reason);
+
/* remove this object from the cache
* BillS Asks.. Why do we need to make this call to remove_url?
* leave it in for now..
*/
- cache_remove_url(r, url);
+ /* Check if there is a previous request and if it has the same
+ * status code. If yes, it is assumed that we are filtering on
+ * an custom error response. But we want to remove the original
+ * entry from cache, so take the data from previous request for
+ * cache_remove_url.
+ */
+ if (r->prev != NULL) {
+ request_rec *prev_r = r->prev;
+
+ if (r->status == prev_r->status) {
+ cache_remove_url(prev_r, prev_r->unparsed_uri);
+ }
+ }
+ else {
+ cache_remove_url(r, url);
+ }
+
/* remove this filter from the chain */
+ apr_table_unset(r->notes, CACHE_SAVE_FILTER_INSERTED);
ap_remove_output_filter(f);
/* ship the data up the stack */
@@ -538,6 +560,7 @@
if (rv != OK) {
/* Caching layer declined the opportunity to cache the response */
+ apr_table_unset(r->notes, CACHE_SAVE_FILTER_INSERTED);
ap_remove_output_filter(f);
return ap_pass_brigade(f->next, in);
}
@@ -705,6 +728,7 @@
rv = cache->provider->store_body(cache->handle, r, in);
}
if (rv != APR_SUCCESS) {
+ apr_table_unset(r->notes, CACHE_SAVE_FILTER_INSERTED);
ap_remove_output_filter(f);
}
@@ -964,6 +988,24 @@
return OK;
}
+/*
+ * Make sure our error-path filter is in place.
+ */
+static void ap_cache_insert_error_filter(request_rec *r)
+{
+ const char *notes;
+
+ notes = apr_table_get(r->notes, CACHE_SAVE_FILTER_INSERTED);
+ /* if cache_save_filter has been in the original request just add
+ it again */
+ if (notes != NULL) {
+ ap_add_output_filter_handle(cache_save_filter_handle, NULL, r,
+ r->connection);
+
+ }
+}
+
+
static const command_rec cache_cmds[] =
{
/* XXX
@@ -1031,6 +1073,9 @@
NULL,
AP_FTYPE_CONTENT_SET-1);
ap_hook_post_config(cache_post_config, NULL, NULL, APR_HOOK_REALLY_FIRST);
+ ap_hook_insert_error_filter(ap_cache_insert_error_filter,
+ NULL, NULL, APR_HOOK_MIDDLE);
+
}
module AP_MODULE_DECLARE_DATA cache_module =
Index: httpd-trunk/modules/cache/mod_cache.h
===================================================================
--- httpd-trunk/modules/cache/mod_cache.h (revision 178625)
+++ httpd-trunk/modules/cache/mod_cache.h (working copy)
@@ -103,6 +103,8 @@
#define CACHE_DECLARE_DATA __declspec(dllimport)
#endif
+#define CACHE_SAVE_FILTER_INSERTED "CACHE_SAVE_FILTER_INSERTED"
+
struct cache_enable {
const char *url;
const char *type;
@@ -197,7 +199,7 @@
const char *urlkey, apr_off_t len);
int (*open_entity) (cache_handle_t *h, request_rec *r,
const char *urlkey);
- int (*remove_url) (const char *urlkey);
+ int (*remove_url) (const char *urlkey, request_rec *r);
} cache_provider;
/* A linked-list of authn providers. */
Index: httpd-trunk/modules/cache/mod_disk_cache.c
===================================================================
--- httpd-trunk/modules/cache/mod_disk_cache.c (revision 178625)
+++ httpd-trunk/modules/cache/mod_disk_cache.c (working copy)
@@ -354,9 +354,56 @@
return OK;
}
-static int remove_url(const char *key)
+static int remove_url(const char *key, request_rec *r)
{
- /* XXX: Delete file from cache! */
+ apr_status_t rc;
+ int error = 0;
+ static int error_logged = 0;
+ disk_cache_conf *conf = ap_get_module_config(r->server->module_config,
+ &disk_cache_module);
+ disk_cache_object_t *dobj;
+
+ /* Check if cache_root is configured */
+ if (conf->cache_root == NULL) {
+ if (!error_logged) {
+ error_logged = 1;
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
+ "disk_cache: Cannot delete files from disk without a
CacheRoot specified.");
+ }
+ return DECLINED;
+ }
+
+ /* Create and init the disk cache object */
+ dobj = apr_pcalloc(r->pool, sizeof(disk_cache_object_t));
+ dobj->name = (char *) key;
+
+ /* Get filenames with full path for data file and headers file */
+ dobj->datafile = data_file(r->pool, conf, dobj, key);
+ dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
+
+ /* Delete data file */
+ rc = apr_file_remove(dobj->datafile, r->pool);
+ if ((rc != APR_SUCCESS) && (rc != APR_ENOENT)) {
+ ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
+ "disk_cache: Failed to delete data file %s from cache.",
+ dobj->datafile);
+ error = 1;
+ }
+
+ /* Delete headers file */
+ rc = apr_file_remove(dobj->hdrsfile, r->pool);
+ if ((rc != APR_SUCCESS) && (rc != APR_ENOENT)) {
+ ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
+ "disk_cache: Failed to delete headers file %s from
cache.",
+ dobj->hdrsfile);
+ error = 1;
+ }
+
+ /* return DECLINED in case of any error except file not found */
+ if (error) {
+ return DECLINED;
+ }
+
return OK;
}
Index: httpd-trunk/modules/cache/cache_storage.c
===================================================================
--- httpd-trunk/modules/cache/cache_storage.c (revision 178625)
+++ httpd-trunk/modules/cache/cache_storage.c (working copy)
@@ -45,7 +45,7 @@
/* for each specified cache type, delete the URL */
while(list) {
- list->provider->remove_url(key);
+ list->provider->remove_url(key, r);
list = list->next;
}
return OK;
