Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Justin Erenkrantz wrote: --On August 11, 2005 10:21:37 AM +0100 Colm MacCarthaigh [EMAIL PROTECTED] wrote: On Wed, Aug 10, 2005 at 03:38:17PM -0700, Justin Erenkrantz wrote: --On August 8, 2005 1:25:46 PM +0100 Colm MacCarthaigh [EMAIL PROTECTED] wrote: O.k., I've merged our two patches, but I've changed a few things, tell me if there's anothing you think is wrong; Would you mind writing up a log message for this patch? No problem; Fix incorrectly served 304 responses when expired cache entity is valid, but cache is unwritable and headers cannot be updated. Submitted by colm stdlib.net Remove entities from the cache when re-validation receives a 404 or other content-no-longer-present error. Submitted by ruediger.pluem vodafone.com Thanks! I've committed this in r231486, r231487, and r231488. I re-split them up to make it easier for people to review the commits. However, there remains an issue in mod_disk_cache's remove_url: I don't think it properly handles removing the Vary condition files. I went ahead and committed it because it gets us closer to the desired solution. The code will remove the header file and the disk file; but it also likely needs to go up a 'level' and remove all variants. Because if we get a 404 on a varied entity, it also means that all variants should be removed, no? I think what this means is that we need to recompute the hash - verify that it either meets the 'base' URL (we're done then), or we need to then go through and wack all of the varied headers/bodies and such. Now, I *think* it's possible after Paul's latest rewrite to just wack some subdirectories - but I'm fuzzy on this. (Paul??) No need to recompute the hash. Just rm -rf hash.vary, which is a directory. Inside that directory is another layer of hashes, but thats only based on the varied headers... Anyways, best path would be to atomically rename the .vary to a tempdir, and then recursively delete it. -Paul
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
On Fri, Aug 12, 2005 at 05:38:40AM +0200, Plm, Rdiger, VIS wrote: In the case that you are caching a response from a backend app server or a cgi script I can imagine situations where one variant is 404 and another one is not. Dw also pointed that out. From my personal point of view we should keep them and let the next revalidation on them caused by a client decide whether they should be removed or not. I guess I just don't buy that as a legitimate (and compliant) use case; but if that's how some servers work, I guess. So, that should mean that the code is fine as-is. Yes, I think we just need to remove the .vary subdirectory with all its subdirectories and files. Right. I think Paul mentioned that we also need to fix up htcacheclean to remove the .vary subdirectories as well. -- justin
htcacheadmin was: Re: [PATCH] fix incorrect 304's responses when cache is unwritable
On Thu, Aug 11, 2005 at 11:48:21PM -0700, Justin Erenkrantz wrote:
Right. I think Paul mentioned that we also need to fix up htcacheclean to
remove the .vary subdirectories as well. -- justin
Next time someone is commiting to htcacheclean; it's define's for
VARY_FORMAT_VERSION and DISK_FORMAT_VERSION are wrong. They should be 3,
and 4 respectively - as per mod_disk_cache.c. Right now they are 1 and
2.
I'm writing something else now, to help me debug cache edge cases, I'm
still seeing some misbehaviour that I need to track down, but I'm having
trouble re-creating vary caching.
If the vary content is locally generated, it is saved as per content
location. And right now, I can't get it cache proxied vary content at
all. Though it doesn't help that I've hamfisting the mod_proxy code in
trunk to even compile.
Does anyone have a remote URI on some webserver somewhere that reliably
returns a Vary response that is cacheable?
That something else; htcacheadmin
Now that I'm running with an expanded cache, and trying to debug things
while I'm at it, I keep coming accross needing to do the same tasks, but
find ./ -type f | xargs grep just isn't a reliable way of tracking
down cache entities. So I've written htcacheadmin.
It's *extremely* useful for debugging, which is why I'm posting it now -
it's helped me a lot. It's useful for administrators too, but I'm not
sure if it's useful enough Vs confusing enough for support/
Anyway, right now it's got most of its functionality working, but it
only works for locally generated non-vary content. I'm going to add a
loop to allow it to work through the various key permutations for proxy
support, eg.
ftp.heanet.ie/pub/heanet/100.txt?
ftp.heanet.iehttp://ftp.heanet.ie/pub/heanet/100.txt?
ftp.heanet.iehttp://ftp.heanet.ie:80/pub/heanet/100.txt?
to track down likely cache matches. Though I wonder is there a hope of
convinving anyone to change the cache key to something utterly
determinstic like;
http://ftp.heanet.ie:80/pub/heanet/100.txt?
including the scheme will help me when I go to make caching work for
ftp, and the rest just helps make it reliably deterministic.
Anyway, other features I want to add including allowing the
administrator to extend the expiry of particular entities, and I think I
might split it's functionality so that it has one mode which locates
(and only locates) cache entities, and then another mode which does the
information retrieval taking a .header file as an argument (more useful
to admins since they can guarantee the output is for one instance only).
See attachment, feedback appreciated.
--
Colm MacCárthaighPublic Key: [EMAIL PROTECTED]
/* Copyright 2001-2005 The Apache Software Foundation or its licensors, as
* applicable.
*
* Licensed under the Apache License, Version 2.0 (the License);
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an AS IS BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/*
* htcacheadmin.c: a utility to allow administrators to track down urls
* in their caches, and perform actions on that basis.
*
* Contributed by Colm MacCarthaigh colm stdlib.net
* 11 Aug 2005
*/
#include apr.h
#include apr_lib.h
#include apr_strings.h
#include apr_file_io.h
#include apr_file_info.h
#include apr_pools.h
#include apr_md5.h
#include apr_getopt.h
#include apr_date.h
#include apr_uri.h
#if APR_HAVE_UNISTD_H
#include unistd.h
#endif
#if APR_HAVE_STDLIB_H
#include stdlib.h
#endif
/* mod_disk_cache.c extract start */
#define VARY_FORMAT_VERSION 3
#define DISK_FORMAT_VERSION 4
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. */
apr_size_t name_len;
/* The number of times we've cached this entity. */
apr_size_t entity_version;
/* Miscellaneous time values. */
apr_time_t date;
apr_time_t expire;
apr_time_t request_time;
apr_time_t response_time;
} disk_cache_info_t;
/* mod_disk_cache.c extract end */
/* cache_util.c extract started */
static void cache_hash(const char *it, char *val, int ndepth, int nlength)
{
apr_md5_ctx_t context;
unsigned char digest[16];
char tmp[22];
int i, k, d;
unsigned int x;
static const char enc_table[64] =
ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_@;
apr_md5_init(context);
apr_md5_update(context, (const unsigned char *) it, strlen(it));
apr_md5_final(digest, context);
/*
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Justin Erenkrantz wrote: On Fri, Aug 12, 2005 at 05:38:40AM +0200, Plm, Rdiger, VIS wrote: In the case that you are caching a response from a backend app server or a cgi script I can imagine situations where one variant is 404 and another one is not. Dw also pointed that out. From my personal point of view we should keep them and let the next revalidation on them caused by a client decide whether they should be removed or not. I guess I just don't buy that as a legitimate (and compliant) use case; but if that's how some servers work, I guess. Agreed. I confess that I sometimes misuse mod_cache to get the performance of odd commercial web applications fixed. And it is easier to misuse mod_cache and sometimes to patch it for this misuse then to get commercial vendors fix their sloppy software :-(. So, that should mean that the code is fine as-is. Basicly yes, from my personal point of view. BTW: Can you have a look at the patch I proposed at http://mail-archives.apache.org/mod_mbox/httpd-dev/200508.mbox/[EMAIL PROTECTED] to delete the empty directories (dir_removal_patch.diff) for the cache entries that get removed? That would be very nice. [..cut..] Thanks Rüdiger
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
--On August 8, 2005 9:46:52 PM +0200 [EMAIL PROTECTED] wrote: log_correction.diff: ... dir_removal_patch.diff: Committed in r232335 and r232334, respectively. Thanks! -- justin
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
On Wed, Aug 10, 2005 at 03:38:17PM -0700, Justin Erenkrantz wrote: --On August 8, 2005 1:25:46 PM +0100 Colm MacCarthaigh [EMAIL PROTECTED] wrote: O.k., I've merged our two patches, but I've changed a few things, tell me if there's anothing you think is wrong; Would you mind writing up a log message for this patch? No problem; Fix incorrectly served 304 responses when expired cache entity is valid, but cache is unwritable and headers cannot be updated. Submitted by colm stdlib.net Remove entities from the cache when re-validation receives a 404 or other content-no-longer-present error. Submitted by ruediger.pluem vodafone.com -- Colm MacCárthaighPublic Key: [EMAIL PROTECTED]
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
--On August 11, 2005 10:21:37 AM +0100 Colm MacCarthaigh [EMAIL PROTECTED] wrote: On Wed, Aug 10, 2005 at 03:38:17PM -0700, Justin Erenkrantz wrote: --On August 8, 2005 1:25:46 PM +0100 Colm MacCarthaigh [EMAIL PROTECTED] wrote: O.k., I've merged our two patches, but I've changed a few things, tell me if there's anothing you think is wrong; Would you mind writing up a log message for this patch? No problem; Fix incorrectly served 304 responses when expired cache entity is valid, but cache is unwritable and headers cannot be updated. Submitted by colm stdlib.net Remove entities from the cache when re-validation receives a 404 or other content-no-longer-present error. Submitted by ruediger.pluem vodafone.com Thanks! I've committed this in r231486, r231487, and r231488. I re-split them up to make it easier for people to review the commits. However, there remains an issue in mod_disk_cache's remove_url: I don't think it properly handles removing the Vary condition files. I went ahead and committed it because it gets us closer to the desired solution. The code will remove the header file and the disk file; but it also likely needs to go up a 'level' and remove all variants. Because if we get a 404 on a varied entity, it also means that all variants should be removed, no? I think what this means is that we need to recompute the hash - verify that it either meets the 'base' URL (we're done then), or we need to then go through and wack all of the varied headers/bodies and such. Now, I *think* it's possible after Paul's latest rewrite to just wack some subdirectories - but I'm fuzzy on this. (Paul??) Does this make sense? Or, am I missing something? -- justin
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
In a message dated 8/11/2005 12:42:35 PM Central Daylight Time, [EMAIL PROTECTED] writes: The code will remove the header file and the disk file; but it also likely needs to go up a 'level' and remove all variants. Because if we get a 404 on a varied entity, it also means that all variants should be removed, no? Actually, no. What you really should do if you are going to drop into 'cleanup' mode on this base URI is RE-VALIDATE ALL THE VARIANTS and remove any that return bad response codes but keep the ones that are 'still ok'. It is possible ( and legal? ) for a COS ( Content Origin Server ) to return a '404' on one variant of an entity but not on another. I have seen CGI scripts that would do this very thing. Yours... Kevin Kiley
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
On Thu, 11 Aug 2005 [EMAIL PROTECTED] wrote: It is possible ( and legal? ) for a COS ( Content Origin Server ) to return a '404' on one variant of an entity but not on another. Regardless - it is quite a common thing to do. Dw.
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
[..cut..] Thanks! I've committed this in r231486, r231487, and r231488. I re-split them up to make it easier for people to review the commits. However, there remains an issue in mod_disk_cache's remove_url: I don't think it properly handles removing the Vary condition files. I went ahead and committed it because it gets us closer to the desired solution. The code will remove the header file and the disk file; but it also likely needs to go up a 'level' and remove all variants. Because if we get a 404 on a varied entity, it also means that all variants should be removed, no? Should they really be removed? In the case that you are caching a response from a backend app server or a cgi script I can imagine situations where one variant is 404 and another one is not. Dw also pointed that out. From my personal point of view we should keep them and let the next revalidation on them caused by a client decide whether they should be removed or not. I think what this means is that we need to recompute the hash - verify that it either meets the 'base' URL (we're done then), or we need to then go through and wack all of the varied headers/bodies and such. Now, I *think* it's possible after Paul's latest rewrite to just wack some subdirectories - but I'm fuzzy on this. (Paul??) Yes, I think we just need to remove the .vary subdirectory with all its subdirectories and files. Does this make sense? Or, am I missing something? -- justin Regards Rüdiger
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
--On August 8, 2005 1:25:46 PM +0100 Colm MacCarthaigh [EMAIL PROTECTED] wrote: O.k., I've merged our two patches, but I've changed a few things, tell me if there's anothing you think is wrong; Would you mind writing up a log message for this patch? I've lost track of what it's supposed to do. ;-) Thanks! -- justin
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
--On August 8, 2005 1:25:46 PM +0100 Colm MacCarthaigh [EMAIL PROTECTED] wrote: O.k., I've merged our two patches, but I've changed a few things, tell me if there's anothing you think is wrong; Would you mind writing up a log message for this patch? I've lost track of what it's supposed to do. ;-) I try to sort things out. Colm please correct me if I mix things up: Colm merged his and my patch which solved the following issues: Colms patch: If the headers for a revalidated cache entry cannot be stored for whatever reason then remove this cache entry from the cache rather then to try revalidating it each time. Cite from the comments of Colms patch: /* Before returning we need to handle the possible case of an * unwritable cache. Rather than leaving the entity in the cache * and having it constantly re-validated, now that we have recalled * the body it is safe to try and remove the url from the cache. */ My patch: It tries to delete cache entries for which a non cacheable response (most focused on 404's) was delivered during revalidation. Basicly this was already implemented, but - the remove_url function of mod_disk_cache was empty - it turned out that error response sometimes bypass the CACHE_SAVE filter (canned error responses) or do not call it with the original request. Those things are fixed by my patch which I developed during ApacheCon after the discussion with you, Paul and Sander. Colms patch relies on the implementation of remove_url in mod_disk_cache to get the file really deleted. Later on I send a patch to the merged patch which - Moves some logging messages which produces misleading messages - Also removes the directory structure of the cached files as far as possible. As Colm pointed out correctly leaving the empty directories on the disk is a pain for the filesystem, so they should be removed. Don't hesitate if you have further question or like to have this patch broken in different chunks for better reviewing / commiting :-). Regards Rüdiger
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Colm MacCarthaigh wrote: On Mon, Aug 08, 2005 at 12:45:21AM +0200, [EMAIL PROTECTED] wrote: Is a traversal really needed? What about going back the full path of the header / data file to the cache root and removing each component on the way by calling apr_dir_remove on each component until it fails? I'm not sure if apr_dir_remove guarantees failure when operated on non-empty directories. If it does then that's an easy enough way. I should be at least on Unix because it simply calls rmdir and this returns ENOTEMPTY on the removal of non empty directories. This results in apr_dir_remove returning APR_ENOTEMPTY. The only problem I see with the current state of my patch is to get the information of the cache_root passed to remove_url. I think this is needed to have a clear stop signal where to stop deleting empty directories at least. Otherwise you may run until / in the worst case :-). [..cut..] Makes sense, O.k., now looking at it and knowing what it is supposed to do, it looks fine. The only things I've noticed are; the obviously mis-copied CACHE_SAVE coment in cache_remove_url_filter() :-) :-) The extraneous -e debug comments in mod_disk_cache I added them because it took me some time to figure out that setting loglevel to debug is not enough to get these messages printed. In mod_disk_cache, you try to delete the data file even if removing the header file was unsuccesful. Personally I wouldn't be very comfortable with this, as the header is a useful source of information to an adminstrator tracking down problems and it's only easy way to determine what the data file is. If you can't delete the header file, I'd recommend leaving the data file in-place. They make more sense if they are both in the same state. Agreed. Feel free to adjust this. In cache_remove_url, you have code which tries to determine if the cache-handle or cache-stale_handle should be removed, but shouldn't it always be the stale_handle? You only add the remove_url filter if cache_select_url() != OK, which means cache-handle will always be NULL. You are right. In the current usage of cache_remove_url this check does not make sense, but I wanted to keep the scope of this function somewhat broder to allow to call this function in the case the caller wants to remove the entity behind cache-handle. But apart from those looks fine. I'll merge it with my small patch and test it properly tomorrow. Looking forward to this. Please let us know the results. Regards Rüdiger
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
On Mon, Aug 08, 2005 at 10:33:47AM +0200, [EMAIL PROTECTED] wrote:
Is a traversal really needed? What about going back the full path of the
header / data file to the cache root and removing each component on the
way by calling apr_dir_remove on each component until it fails?
I'm not sure if apr_dir_remove guarantees failure when operated on
non-empty directories. If it does then that's an easy enough way.
I should be at least on Unix because it simply calls rmdir and this returns
ENOTEMPTY on the removal of non empty directories. This results in
apr_dir_remove returning APR_ENOTEMPTY.
I'm just being paranoid, but I'm not certain of the behaviour of the
win32, beos or other future implementations, and the APR itself makes no
claims about apr_dir_remove's behaviour - it just says Remove directory
from the file system which is pretty ambiguous. I've now checked the
win32 and beos implementations, and they seem safe, so I've submitted a
silly doxygen comment patch to [EMAIL PROTECTED] to try and get rid of the
problem
for good.
In cache_remove_url, you have code which tries to determine if
the cache-handle or cache-stale_handle should be removed, but
shouldn't it always be the stale_handle? You only add the
remove_url filter if cache_select_url() != OK, which means
cache-handle will always be NULL.
You are right. In the current usage of cache_remove_url this
check does not make sense, but I wanted to keep the scope of
this function somewhat broder to allow to call this function
in the case the caller wants to remove the entity behind
cache-handle.
x. At the very least, I'd definitely make stale_handle the default if
both pointers are non-NULL :)
O.k., I've merged our two patches, but I've changed a few things, tell
me if there's anothing you think is wrong;
1. Did some minor whitespace/style cleanups, nothing
big. also changed the instances of CACHE_REMOVAL_URL
in comments to CACHE_REMOVE_URL, and a few small things like
that cought. Also made the log messages consistent
with the rest of mod_cache, and added some to aid
testing.
2. modified the mod_disk_cache code to delete the data only
if the header could be deleted first.
3. renamed the cache_remove_url_filter pointer in the cache
struct to remove_url_filter, cache-cache_$something seemed
redundant and it makes the formatting neater :-)
4. Fixed a bug in the mod_disk_cache code. It was using
dobj-datafile in the dobj-hdrsfile section for log
output.
5. Swapped the order of the ternary check in cache_storage,
so that we'll remove the stale_handle if both the stale_hand
and handle are non-NULL.
The patch is attached, and I've been testing it for the last few hours
without problem. The code is now running on ftp.heanet.ie. (along
with htcacheclean -t).
--
Colm MacCárthaighPublic Key: [EMAIL PROTECTED]
Index: modules/cache/mod_mem_cache.c
===
--- modules/cache/mod_mem_cache.c (revision 230717)
+++ 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(cache_handle_t *h, apr_pool_t *p)
{
cache_object_t *obj;
int cleanup = 0;
@@ -609,8 +609,8 @@
if (sconf-lock) {
apr_thread_mutex_lock(sconf-lock);
}
-
-obj = cache_find(sconf-cache_cache, key);
+
+obj = h-cache_obj;
if (obj) {
cache_remove(sconf-cache_cache, obj);
/* For performance, cleanup cache object after releasing the lock */
Index: modules/cache/mod_cache.c
===
--- modules/cache/mod_cache.c (revision 230717)
+++ modules/cache/mod_cache.c (working copy)
@@ -29,6 +29,7 @@
*/
static ap_filter_rec_t *cache_save_filter_handle;
static ap_filter_rec_t *cache_out_filter_handle;
+static ap_filter_rec_t *cache_remove_url_filter_handle;
/*
* CACHE handler
@@ -123,6 +124,19 @@
/* add cache_save filter to cache this request */
ap_add_output_filter_handle(cache_save_filter_handle, NULL, r,
r-connection);
+
+ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r-server,
+ Adding CACHE_REMOVE_URL filter.);
+
+/* Add cache_remove_url filter to this request to remove a
+ * stale cache entry if needed. Also put the current cache
+ * request rec in the filter context, as the request that
+ * is available later during running the filter maybe
+ * different due to an internal redirect.
+ */
+cache-remove_url_filter =
+
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Colm MacCarthaigh wrote: On Mon, Aug 08, 2005 at 10:33:47AM +0200, [EMAIL PROTECTED] wrote: [..cut..] The patch is attached, and I've been testing it for the last few hours without problem. The code is now running on ftp.heanet.ie. (along with htcacheclean -t). Looks very good to me. Only two minor typos :-): 1. remove a confirmed stale cache One space too much between confirmed and stale 2. non-stalle handle should be non-stalled handle Regards Rüdiger
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Andreas Steinmetz said: The problem is that you can't remove directories with htcacheclean without generating race conditions wrt. httpd. In this case the race in httpd should be fixed. In theory, httpd should attempt to create the directory, then attempt to move the file to that directory. If the move fails, attempt to create a directory and move the file again, until successful or some sane max retries, in which case give up and don't cache the file. Regards, Graham --
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Colm MacCarthaigh wrote:
On Mon, Aug 08, 2005 at 10:33:47AM +0200, [EMAIL PROTECTED] wrote:
[..cut..]
O.k., I've merged our two patches, but I've changed a few things, tell
me if there's anothing you think is wrong;
I attached two further patches to your merged patch:
log_correction.diff:
This moves the debuging log message about the removal of a url from
cache_remove_url_filter
to cache_remove_url.
Reason: It is not a good idea to use f-r-unparsed_uri for the log message as
this
will display not the url that gets removed from the cache, but the url of the
error document
(at least in some case) and thus is misleading.
Therefore I moved it to cache_remove_url and take h-cache_obj-key instead for
the debug message.
dir_removal_patch.diff:
This patch will remove the directory path of the cached file as far as possible
to get rid
of unused empty directories. And yes, I trust apr_dir_remove :-).
[..cut..]
Regards
Rüdiger
diff -Nrup httpd-trunk.patched/modules/cache/mod_disk_cache.c
httpd-trunk/modules/cache/mod_disk_cache.c
--- httpd-trunk.patched/modules/cache/mod_disk_cache.c 2005-08-08
16:53:08.0 +0200
+++ httpd-trunk/modules/cache/mod_disk_cache.c 2005-08-08 20:36:31.0
+0200
@@ -370,6 +370,8 @@ static int create_entity(cache_handle_t
dobj-name = obj-key;
dobj-prefix = NULL;
+/* Save the cache root */
+dobj-root = apr_pstrdup(r-pool, conf-cache_root);
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);
@@ -413,6 +415,9 @@ static int open_entity(cache_handle_t *h
/* Open the headers file */
dobj-prefix = NULL;
+/* Save the cache root */
+dobj-root = apr_pstrdup(r-pool, conf-cache_root);
+
dobj-hdrsfile = 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);
@@ -518,6 +523,10 @@ static int remove_url(cache_handle_t *h,
{
apr_status_t rc;
disk_cache_object_t *dobj;
+const char *str_to_copy;
+char *dir;
+char *slash;
+char *q;
/* Get disk cache object from cache handle */
dobj = (disk_cache_object_t *) h-cache_obj-vobj;
@@ -559,6 +568,33 @@ static int remove_url(cache_handle_t *h,
}
}
+/* now delete directories as far as possible */
+if (dobj-root) {
+
+str_to_copy = dobj-hdrsfile ? dobj-hdrsfile : dobj-datafile;
+if (str_to_copy) {
+dir = apr_pstrdup(p, str_to_copy);
+
+/* remove filename */
+slash = strrchr(dir, '/');
+*slash = '\0';
+/*
+ * now walk our way back to the cache root, delete everything
+ * in the way as far as possible
+ */
+for (q = dir + strlen(dobj-root); *q ; ) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
+ disk_cache: Deleting directory %s from cache.,
dir);
+ rc = apr_dir_remove(dir, p);
+ if (rc != APR_SUCCESS !APR_STATUS_IS_ENOENT(rc)) {
+break;
+ }
+ slash = strrchr(q, '/');
+ *slash = '\0';
+}
+}
+}
+
return OK;
}
diff -Nrup httpd-trunk.patched/modules/cache/cache_storage.c
httpd-trunk/modules/cache/cache_storage.c
--- httpd-trunk.patched/modules/cache/cache_storage.c 2005-08-08
16:53:08.0 +0200
+++ httpd-trunk/modules/cache/cache_storage.c 2005-08-08 20:35:07.0
+0200
@@ -46,7 +46,8 @@ int cache_remove_url(cache_request_rec *
if (!h) {
return OK;
}
-
+ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
+ cache: Removing url %s from the cache,
h-cache_obj-key);
/* for each specified cache type, delete the URL */
while(list) {
list-provider-remove_url(h, p);
diff -Nrup httpd-trunk.patched/modules/cache/mod_cache.c
httpd-trunk/modules/cache/mod_cache.c
--- httpd-trunk.patched/modules/cache/mod_cache.c 2005-08-08
16:53:08.0 +0200
+++ httpd-trunk/modules/cache/mod_cache.c 2005-08-08 20:43:59.0
+0200
@@ -802,8 +802,6 @@ static int cache_remove_url_filter(ap_fi
/*
* Now remove this cache entry from the cache
*/
-ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r-server,
- cache: Removing url %s from the cache, f-r-unparsed_uri);
cache_remove_url(cache, r-pool);
/* remove ourselves */
ap_remove_output_filter(f);
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Graham Leggett wrote:
Andreas Steinmetz said:
The problem is that you can't remove directories with htcacheclean
without generating race conditions wrt. httpd.
In this case the race in httpd should be fixed.
In theory, httpd should attempt to create the directory, then attempt to
move the file to that directory. If the move fails, attempt to create a
directory and move the file again, until successful or some sane max
retries, in which case give up and don't cache the file.
Fix attached and compile tested against 2.1.6 alpha.
--
Andreas Steinmetz SPAMmers use [EMAIL PROTECTED]
--- httpd-2.1.6-alpha/modules/cache/mod_disk_cache.c.orig 2005-08-08
21:52:24.0 +0200
+++ httpd-2.1.6-alpha/modules/cache/mod_disk_cache.c2005-08-08
22:12:14.0 +0200
@@ -736,6 +736,7 @@
disk_cache_info_t disk_info;
struct iovec iov[2];
+int i;
/* This is flaky... we need to manage the cache_info differently */
h-cache_obj-info = *info;
@@ -774,7 +775,14 @@
dobj-tfd = NULL;
-rv = apr_file_rename(dobj-tempfile, dobj-hdrsfile, r-pool);
+for (i = 0; i 3; i++) {
+rv = apr_file_rename(dobj-tempfile, dobj-hdrsfile, r-pool);
+if (rv == APR_SUCCESS) {
+break;
+}
+apr_sleep(1000);
+mkdir_structure(conf, dobj-hdrsfile, r-pool);
+}
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r-server,
disk_cache: rename tempfile to varyfile failed: %s - %s,
@@ -865,7 +873,14 @@
mkdir_structure(conf, dobj-hdrsfile, r-pool);
}
-rv = apr_file_rename(dobj-tempfile, dobj-hdrsfile, r-pool);
+for (i = 0; i 3; i++) {
+rv = apr_file_rename(dobj-tempfile, dobj-hdrsfile, r-pool);
+if (rv == APR_SUCCESS) {
+break;
+}
+apr_sleep(1000);
+mkdir_structure(conf, dobj-hdrsfile, r-pool);
+}
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, rv, r-server,
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Colm MacCarthaigh wrote:
I finally developed some time to look into this. mod_cache doesn't
behave very nicely when the cache area fills. Of course administators
should make sure it doesn't fill in the first place, but nevertheless a
few people have hit this bug (me included) and I think mod_cache should
handle the problem gracefully.
Anyway, the problem occurs when the cache is unwritable, and mod_cache
needs to revalidate a cached entity. cache_select_url handles this by
rewriting headers_in to become a conditional request. However the code
in cache_save_filter which turns the request back into its original
(possibly unconditional) format is itself conditional on store_headers()
working.
The patch I've attached should be reasonably self-documenting, any
questions - just ask.
As you already mentioned the remove_url implementation
of mod_disk_cache is currently an empty dummy :-).
I had a similar problem with 404 responses, and wrote a patch for this which is
currently in discussion (attached patch again to this mail):
http://mail-archives.apache.org/mod_mbox/httpd-dev/200507.mbox/[EMAIL PROTECTED]
It actually does implement a removal of the files in mod_disk_cache and
should also handle your problem. If it does not, I am pretty sure that a small
modification
to the patch would do it.
I would really appreciate if you find some time to review my patch.
Thanks and regards
Rüdiger
Index: modules/cache/mod_mem_cache.c
===
--- modules/cache/mod_mem_cache.c (Revision 220022)
+++ modules/cache/mod_mem_cache.c (Arbeitskopie)
@@ -601,7 +601,7 @@
/* remove_url()
* Notes:
*/
-static int remove_url(const char *key)
+static int remove_url(cache_handle_t *h, apr_pool_t *p)
{
cache_object_t *obj;
int cleanup = 0;
@@ -609,8 +609,8 @@
if (sconf-lock) {
apr_thread_mutex_lock(sconf-lock);
}
-
-obj = cache_find(sconf-cache_cache, key);
+
+obj = h-cache_obj;
if (obj) {
cache_remove(sconf-cache_cache, obj);
/* For performance, cleanup cache object after releasing the lock */
Index: modules/cache/mod_cache.c
===
--- modules/cache/mod_cache.c (Revision 220022)
+++ modules/cache/mod_cache.c (Arbeitskopie)
@@ -29,6 +29,7 @@
*/
static ap_filter_rec_t *cache_save_filter_handle;
static ap_filter_rec_t *cache_out_filter_handle;
+static ap_filter_rec_t *cache_remove_url_filter_handle;
/*
* CACHE handler
@@ -123,6 +124,22 @@
/* add cache_save filter to cache this request */
ap_add_output_filter_handle(cache_save_filter_handle, NULL, r,
r-connection);
+
+ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r-server,
+ Adding CACHE_REMOVE_URL filter.);
+
+/*
+ * add cache_remove_url filter to this request to remove the
+ * cache entry if it is needed. Store the filter in the cache
+ * request rec for easy removal if it turns out that we do not
+ * need it, because we are caching it. Also put the current
+ * cache request rec in the filter context, as the request that
+ * is available later during running the filter maybe
+ * different due to an internal redirect.
+ */
+cache-cache_remove_url_filter =
+
ap_add_output_filter_handle(cache_remove_url_filter_handle, cache, r,
+ r-connection);
}
else if (cache-stale_headers) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r-server,
@@ -436,11 +453,6 @@
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);
/* remove this filter from the chain */
ap_remove_output_filter(f);
@@ -542,6 +554,15 @@
cache: Caching url: %s, url);
/*
+ * We are actually caching this response. So it does not
+ * make sense to remove this entry in cache_remove_url_filter
+ * So remove it.
+ */
+ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r-server,
+ cache: Removing CACHE_REMOVE_URL filter.);
+ap_remove_output_filter(cache-cache_remove_url_filter);
+
+/*
* We now want to update the cache file header information with
* the new date, last modified, expire and content length and write
* it away to our cache file. First, we determine these values from
@@ -709,6 +730,53 @@
return
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
On Sun, Aug 07, 2005 at 09:59:15PM +0200, [EMAIL PROTECTED] wrote:
As you already mentioned the remove_url implementation
of mod_disk_cache is currently an empty dummy :-).
I've been thinking about that, but it's not entirely as easy as it first
seems, or indeed as htcacheclean wrongly assumes.
unlinking the data/header files is not good enough, directories are also
resources, and consume space in the inode table. Also, most filesystems
slow down as the ammount of hard-links in any directory increases.
The hash function used by mod_cache is 128-bits in size, which given any
conceivable settings for CacheDirLevels and CacheDirLength is still
going to be more than enough to exhaust the inode table on any rational
filesystem. Considering that one of the main uses of mod_cache is for
mod_proxy users, where there is an infinity of keys (urls) this means
that the filesystem will eventually become unusable unless the
adminstator periodically weighs in an removes empty directories
manually.
Because of Windows and non-standard filesystems like AFS (read the GNU
find(1) manpage section on -noleaf) assumptions about the directory link
counts can't be made, which means a full-scale readdir() and directory
traversal to remove the cache entry. This can be pretty a prettly slow
operation. Possibly slow enough to merit implementing it in an
APR_HOOK_REALLY_LAST hook, so that it can avoid slowing down the request
serving.
An alternative which I think was discussed here is to create a cgid-like
process which deals with this kind of task, aswell as more complex
things like managing just-to-expire files. But that kind of steps on the
toes of the cache_requester SoC work.
Either way, implementing an immediate unlink() on the files, just to be
able to return useful things about the writability of the filesystem,
but leave the more complex directory cleanup until the file has been
served is what I'm thinking of.
For what it's worth though, htcacheclean itself has this massive bug,
and does not do any directory cleanup, so your patch isn't alone in
doing this.
I had a similar problem with 404 responses, and wrote a patch for this which
is
currently in discussion (attached patch again to this mail):
http://mail-archives.apache.org/mod_mbox/httpd-dev/200507.mbox/[EMAIL
PROTECTED]
It actually does implement a removal of the files in mod_disk_cache and
should also handle your problem. If it does not, I am pretty sure that a
small modification
to the patch would do it.
I would really appreciate if you find some time to review my patch.
I'm not a committer, so my review is only informational. But I'm
familiar enough with the cache mode, I've been running and patching it
for my own purposes for years, so I've taken a look.
I'm not really sure what it is aiming to do in relation to 404's. 404's
are never saved to the cache in the first place, the check in
mod_cache.c sorts that out;
if (r-status != HTTP_OK r-status != HTTP_NON_AUTHORITATIVE
r-status != HTTP_MULTIPLE_CHOICES
r-status != HTTP_MOVED_PERMANENTLY
r-status != HTTP_NOT_MODIFIED) {
/* RFC2616 13.4 we are allowed to cache 200, 203, 206, 300, 301 or 410
* We don't cache 206, because we don't (yet) cache partial responses.
* We include 304 Not Modified here too as this is the origin server
* telling us to serve the cached copy.
*/
reason = apr_psprintf(p, Response status %d, r-status);
}
Are 404's being served incorrectly in some circumstances?
In any case;
Your remove_url code in mod_disk_cache takes the approach of just
unlinking the data and header file I discussed above. But well, since
so does htcacheclean, that's no reflection on the quality of the patch.
I don't think the cache_removal_url filter is neccessary, the
cache_save_filter already has a lot of code in place for handling
the case of a stale cache handle where I think it is better placed.
The (much smaller) patch I've just submitted, plus your changes to
mod_disk_cache, mod_mem_cache and cache_storage.c would do the same job
with a third of the code :)
--
Colm MacCárthaighPublic Key: [EMAIL PROTECTED]
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Colm MacCarthaigh wrote: For what it's worth though, htcacheclean itself has this massive bug, and does not do any directory cleanup, so your patch isn't alone in doing this. The problem is that you can't remove directories with htcacheclean without generating race conditions wrt. httpd. Assume that htcacheclean removes the last entries from a directory and then removes the directory. At the same time httpd wants to use the directory as it was already there... You'll be better off setting CacheDirLength and CacheDirLevels to sensible values. Try: CacheDirLength 1 CacheDirLevels 2 You can get at most 64^2 directories which is 4096 directories. Any reasonable filesystem can stand that. Now assume an average of 500 cached objects per directory which the filesystem should easily manage. I tend to believe that 2048000 cached objects is quite a lot. If the size of these objects has an average of only 1KB you already have a total cache size of 2GB. Even if you tried 3 for CacheDirLevels it would be 262144 directories maximum which should be still fine for any reasonable filesystem. -- Andreas Steinmetz SPAMmers use [EMAIL PROTECTED]
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Colm MacCarthaigh wrote:
On Sun, Aug 07, 2005 at 09:59:15PM +0200, [EMAIL PROTECTED] wrote:
As you already mentioned the remove_url implementation
of mod_disk_cache is currently an empty dummy :-).
I've been thinking about that, but it's not entirely as easy as it first
seems, or indeed as htcacheclean wrongly assumes.
unlinking the data/header files is not good enough, directories are also
resources, and consume space in the inode table. Also, most filesystems
slow down as the ammount of hard-links in any directory increases.
Indeed a very valid point. So, yes as far as possible all directories should be
removed, when the header and the data files get removed.
But additionally my experience with some filesystem types is that removing
files from a directory is not enough to speed things up, because the directories
itself do not shrink even if entries get removed from the directory
(I noticed this behaviour with ext3 on Linux and ufs / Veritas FS on Solaris,
wheras
reiserfs shrinks). This could pose a problem to the cache root where all the
temporary data files get created before being moved to the correct hashed
directory.
[..cut..]
Because of Windows and non-standard filesystems like AFS (read the GNU
find(1) manpage section on -noleaf) assumptions about the directory link
counts can't be made, which means a full-scale readdir() and directory
traversal to remove the cache entry. This can be pretty a prettly slow
operation. Possibly slow enough to merit implementing it in an
APR_HOOK_REALLY_LAST hook, so that it can avoid slowing down the request
serving.
Is a traversal really needed? What about going back the full path of the
header / data file to the cache root and removing each component on the
way by calling apr_dir_remove on each component until it fails?
[..cut..]
I would really appreciate if you find some time to review my patch.
I'm not a committer, so my review is only informational. But I'm
familiar enough with the cache mode, I've been running and patching it
for my own purposes for years, so I've taken a look.
I also appreciate comments from non-commiters, as
- this will (hopefully) draw the attention of the commiters.
- improves the patch
I'm not really sure what it is aiming to do in relation to 404's. 404's
are never saved to the cache in the first place, the check in
mod_cache.c sorts that out;
if (r-status != HTTP_OK r-status != HTTP_NON_AUTHORITATIVE
r-status != HTTP_MULTIPLE_CHOICES
r-status != HTTP_MOVED_PERMANENTLY
r-status != HTTP_NOT_MODIFIED) {
/* RFC2616 13.4 we are allowed to cache 200, 203, 206, 300, 301 or 410
* We don't cache 206, because we don't (yet) cache partial responses.
* We include 304 Not Modified here too as this is the origin server
* telling us to serve the cached copy.
*/
reason = apr_psprintf(p, Response status %d, r-status);
}
Are 404's being served incorrectly in some circumstances?
You are right that 404's do not get cached. But if a cached resource vanishes
on the backend
the cache entry is not removed. This
- fills up the cache
- leads to the delivery of the cached resource when the request does not force
mod_cache
to revalidate the entry. This is even the case *if* there had been a request
before which
forced mod_cache to revalidate the cache entry and the revalidation found out
that the resource
has vanished on the backend.
In any case;
Your remove_url code in mod_disk_cache takes the approach of just
unlinking the data and header file I discussed above. But well, since
so does htcacheclean, that's no reflection on the quality of the patch.
See my comments above. Your points are very valid. As I do not want to put
too much into a single patch I currently do not intend to adjust this, but
I definitely will investigate this for a second step.
I don't think the cache_removal_url filter is neccessary, the
cache_save_filter already has a lot of code in place for handling
the case of a stale cache handle where I think it is better placed.
It is needed because:
- In the case of an internal Apache 404 error page the content filter chain
is not run (especially not CACHE_SAVE_FILTER). This is the reason why
cache_removal_url is a protocol filter.
- In the case of an user specified error page with ErrorDocument the
CACHE_SAVE_FILTER
is run with the wrong request (the one that belongs to the custom error page,
not
the one of the original request).
[..cut..]
Regards
Rüdiger
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
On Mon, Aug 08, 2005 at 12:07:09AM +0200, Andreas Steinmetz wrote: Colm MacCarthaigh wrote: For what it's worth though, htcacheclean itself has this massive bug, and does not do any directory cleanup, so your patch isn't alone in doing this. The problem is that you can't remove directories with htcacheclean without generating race conditions wrt. httpd. Assume that htcacheclean removes the last entries from a directory and then removes the directory. At the same time httpd wants to use the directory as it was already there... Well that's a pretty each race to solve within httpd. It won't be able to create the headers, or the body. The patch I've submitted cleans up that slight race. The file won't be cached on that serve, but I don't think that's a big deal :-) You'll be better off setting CacheDirLength and CacheDirLevels to sensible values. Try: CacheDirLength 1 CacheDirLevels 2 You can get at most 64^2 directories which is 4096 directories. But when the link count for the directories themselves will get very large indeed, and common filesystems like ext3 or XFS will get pretty slow indeed. The whole point of allowing them to be split up is to avoid this :) Any reasonable filesystem can stand that. Now assume an average of 500 cached objects per directory which the filesystem should easily manage. I tend to believe that 2048000 cached objects is quite a lot. If the size of these objects has an average of only 1KB you already have a total cache size of 2GB. Well I run a cache which is 135Gb, which regularly has nearly 50 million jpegs in it (revisions of satelite imagery of the entire planet, not porn), but my numbers are always insane. For another example; http://ftp.heanet.ie/status/ I turned on the cache only about 2 hours ago (after applying the patch I've sent) and it's already at 537,029 cached files. (though it fills in spurts, as other projects pull from us). Even if you tried 3 for CacheDirLevels it would be 262144 directories maximum which should be still fine for any reasonable filesystem. You're right, and I'm being a bit over the top with my numbers and examples, but I still think there is danger of inode exhaustion in real-world configurations. I've run proxy clusters in the past which handled tens of millions of requests per day. I don't see why a cluster of Apache 2 proxies shouldn't be able to share a network accessible mod_disk_cache cache area, for example. -- Colm MacCárthaighPublic Key: [EMAIL PROTECTED]
Re: [PATCH] fix incorrect 304's responses when cache is unwritable
On Mon, Aug 08, 2005 at 12:45:21AM +0200, [EMAIL PROTECTED] wrote: Is a traversal really needed? What about going back the full path of the header / data file to the cache root and removing each component on the way by calling apr_dir_remove on each component until it fails? I'm not sure if apr_dir_remove guarantees failure when operated on non-empty directories. If it does then that's an easy enough way. Are 404's being served incorrectly in some circumstances? You are right that 404's do not get cached. But if a cached resource vanishes on the backend the cache entry is not removed. Aha, now I understand what this patch is meant to do. It is needed because: - In the case of an internal Apache 404 error page the content filter chain is not run (especially not CACHE_SAVE_FILTER). This is the reason why cache_removal_url is a protocol filter. - In the case of an user specified error page with ErrorDocument the CACHE_SAVE_FILTER is run with the wrong request (the one that belongs to the custom error page, not the one of the original request). Makes sense, O.k., now looking at it and knowing what it is supposed to do, it looks fine. The only things I've noticed are; the obviously mis-copied CACHE_SAVE coment in cache_remove_url_filter() :-) The extraneous -e debug comments in mod_disk_cache In mod_disk_cache, you try to delete the data file even if removing the header file was unsuccesful. Personally I wouldn't be very comfortable with this, as the header is a useful source of information to an adminstrator tracking down problems and it's only easy way to determine what the data file is. If you can't delete the header file, I'd recommend leaving the data file in-place. They make more sense if they are both in the same state. In cache_remove_url, you have code which tries to determine if the cache-handle or cache-stale_handle should be removed, but shouldn't it always be the stale_handle? You only add the remove_url filter if cache_select_url() != OK, which means cache-handle will always be NULL. But apart from those looks fine. I'll merge it with my small patch and test it properly tomorrow. -- Colm MacCárthaighPublic Key: [EMAIL PROTECTED]
