Re: [PATCH] caching and query strings
Kris,
I am in the process of adding virtual host info into the key
generation too. I'll include your work with mine if that's okay.
Paul J. Reder
Kris Verbeeck wrote:
Hi,
Some of our QA people discovered a problem when performing request
with a query string on a mod_cache enabled Apache 2.0.40 setup.
Request 1: /test.html?x=1y=3
Request 2: /test.html?x=2y=4
Performing request 1 triggers mod_cache to store the response in its
cache. When performing request 2, the data from request 1 is returned
because mod_cache only uses the URI when generating a cache key. The
patch that is attached will fix this.
According to RFC 2616 (HTTP/1.1) paragraph 13.9:
...: since some applications have traditionally used GETs
and HEADs with query URLs (those containing a ? in the
rel_path part) to perform operations with significant side
effects, caches MUST NOT treat responses to such URIs as
fresh unless the server provides an explicit expiration
time.
The fix does the following:
1. decline caching when there is a query string present and
no 'Expires' header is found (mod_cache.c)
2. use 'URI' + '?' + 'query string' has the hash key instead
of only the URI (cache_storage.c)
--- cache_storage.c Thu Sep 12 14:05:31 2002
+++ cache_storage.c-PATCHED Thu Sep 12 14:06:18 2002
-294,7 +294,7
apr_status_t cache_generate_key_default( request_rec *r, apr_pool_t*p, char**key )
{
- *key = apr_pstrdup(p,r-uri);
+ *key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
return APR_SUCCESS;
}
--- mod_cache.c Thu Sep 12 14:14:44 2002
+++ mod_cache.c-PATCHED Thu Sep 12 14:14:55 2002
-538,6 +538,10
r-status != HTTP_NOT_MODIFIED)
/* if a broken Expires header is present, don't cache it */
|| (exps != NULL exp == APR_DATE_BAD)
+/* if query string present but no expiration time, don't cache it
+ * (RFC 2616/13.9)
+ */
+|| (r-args exps == NULL)
/* if the server said 304 Not Modified but we have no cache
* file - pass this untouched to the user agent, it's not for us.
*/
cache_storage.c.diff
Content-Type:
text/plain
Content-Encoding:
7BIT
mod_cache.c.diff
Content-Type:
text/plain
Content-Encoding:
7BIT
--
Paul J. Reder
---
The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure.
-- Albert Einstein
RE: [PATCH] caching and query strings
Kris, Thank you for your contribution. I committed the patch to mod_cache.c. Paul Reder is doing some work in the code to generate the search key and will incorporate your patch when he is complete. Bill Hi, Some of our QA people discovered a problem when performing request with a query string on a mod_cache enabled Apache 2.0.40 setup. Request 1: /test.html?x=1y=3 Request 2: /test.html?x=2y=4 Performing request 1 triggers mod_cache to store the response in its cache. When performing request 2, the data from request 1 is returned because mod_cache only uses the URI when generating a cache key. The patch that is attached will fix this. According to RFC 2616 (HTTP/1.1) paragraph 13.9: ...: since some applications have traditionally used GETs and HEADs with query URLs (those containing a ? in the rel_path part) to perform operations with significant side effects, caches MUST NOT treat responses to such URIs as fresh unless the server provides an explicit expiration time. The fix does the following: 1. decline caching when there is a query string present and no 'Expires' header is found (mod_cache.c) 2. use 'URI' + '?' + 'query string' has the hash key instead of only the URI (cache_storage.c) -- ir. Kris Verbeeck Development Engineer Ubizen - Ubicenter - Philipssite 5 - 3001 Leuven - Belgium T: +32 16 28 70 64 F: +32 16 28 70 77 Ubizen - We Secure e-business - www.ubizen.com
Re: [PATCH] caching and query strings
Kris Verbeeck [EMAIL PROTECTED] wrote:
apr_status_t cache_generate_key_default( request_rec *r, apr_pool_t*p,
char**key )
{
- *key = apr_pstrdup(p,r-uri);
+ *key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
return APR_SUCCESS;
}
Hm... This should be something like:
If (r-args) {
*key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
} else {
*key = apr_pstrdup(p,r-uri);
}
But I might be severely stupid...
Pier
Re: [PATCH] caching and query strings
Yes, I believe it should check r-args. I don't think you are stupid,
severely or otherwise... ;)
Pier Fumagalli wrote:
Kris Verbeeck [EMAIL PROTECTED] wrote:
apr_status_t cache_generate_key_default( request_rec *r, apr_pool_t*p,
char**key )
{
- *key = apr_pstrdup(p,r-uri);
+ *key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
return APR_SUCCESS;
}
Hm... This should be something like:
If (r-args) {
*key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
} else {
*key = apr_pstrdup(p,r-uri);
}
But I might be severely stupid...
Pier
--
Paul J. Reder
---
The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure.
-- Albert Einstein
Re: [PATCH] caching and query strings
Paul J. Reder [EMAIL PROTECTED] wrote: Yes, I believe it should check r-args. I don't think you are stupid, severely or otherwise... ;) That's what _you_ think... Others (like me) tend to disagree! :) After thinking about it, it wouldn't really matter, because apr_pstrcat will already stop at the first NULL argument passed... The only problem I see in that approach would be that two requests: http://www.myserver.com/blah? and http://www.myserver.com/blah would end up with the same key blah?\0 if you don't check for that beforehands (but I don't know exactly how the http stack would parse the first request)... I believe this is ignorable... Pier
Re: [PATCH] caching and query strings
On Thu, 12 Sep 2002 03:47:58 -0700, Paul J. Reder wrote:
Yes, I believe it should check r-args. I don't think you are stupid,
severely or otherwise... ;)
it should not make a difference really
if r-args is null than the strcat should terminate there anyway ;-)
on another note..
Paul.. I'm not 100% on that hook for key-generation is working the
way it should. are you still using that or are you going to always use
the query-args/virtual host for every request, or are you going to provide
2 functions to generate the key (a fast/quick and a slow/flexible one?)
Pier Fumagalli wrote:
Kris Verbeeck [EMAIL PROTECTED] wrote:
apr_status_t cache_generate_key_default( request_rec *r, apr_pool_t*p,
char**key )
{
- *key = apr_pstrdup(p,r-uri);
+ *key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
return APR_SUCCESS;
}
Hm... This should be something like:
If (r-args) {
*key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
} else {
*key = apr_pstrdup(p,r-uri);
}
But I might be severely stupid...
Pier
Re: [PATCH] caching and query strings
But I'm also factoring the hostname into key creation, which also might be NULL. So even if the args issue could be ignored, the hostname can't (or at least the possibility of 1 out of 2 NULL can't be ignored). Pier Fumagalli wrote: Paul J. Reder [EMAIL PROTECTED] wrote: Yes, I believe it should check r-args. I don't think you are stupid, severely or otherwise... ;) That's what _you_ think... Others (like me) tend to disagree! :) After thinking about it, it wouldn't really matter, because apr_pstrcat will already stop at the first NULL argument passed... The only problem I see in that approach would be that two requests: http://www.myserver.com/blah? and http://www.myserver.com/blah would end up with the same key blah?\0 if you don't check for that beforehands (but I don't know exactly how the http stack would parse the first request)... I believe this is ignorable... Pier -- Paul J. Reder --- The strength of the Constitution lies entirely in the determination of each citizen to defend it. Only if every single citizen feels duty bound to do his share in this defense are the constitutional rights secure. -- Albert Einstein
Re: [PATCH] caching and query strings
Ian Holsman wrote:
On Thu, 12 Sep 2002 03:47:58 -0700, Paul J. Reder wrote:
Yes, I believe it should check r-args. I don't think you are stupid,
severely or otherwise... ;)
it should not make a difference really
if r-args is null than the strcat should terminate there anyway ;-)
on another note..
Paul.. I'm not 100% on that hook for key-generation is working the
way it should. are you still using that or are you going to always use
the query-args/virtual host for every request, or are you going to provide
2 functions to generate the key (a fast/quick and a slow/flexible one?)
Well, I'm not adding a hook (or optional function). Currently the code
seems to be there to use the optional function if one is defined, but there
isn't one at present. The code seems to drop through to calling
cache_generate_key_default. I would assume that anyone who created and registered
a more complex cache_generate_key_foo function would be responsible for making
sure it functioned in a sane manner.
Pier Fumagalli wrote:
Kris Verbeeck [EMAIL PROTECTED] wrote:
apr_status_t cache_generate_key_default( request_rec *r, apr_pool_t*p,
char**key )
{
- *key = apr_pstrdup(p,r-uri);
+ *key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
return APR_SUCCESS;
}
Hm... This should be something like:
If (r-args) {
*key = apr_pstrcat(p,r-uri, ?, r-args, NULL);
} else {
*key = apr_pstrdup(p,r-uri);
}
But I might be severely stupid...
Pier
--
Paul J. Reder
---
The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure.
-- Albert Einstein
Re: [PATCH] caching and query strings
Ian Holsman wrote: On Thu, 12 Sep 2002 03:47:58 -0700, Paul J. Reder wrote: Yes, I believe it should check r-args. I don't think you are stupid, severely or otherwise... ;) it should not make a difference really if r-args is null than the strcat should terminate there anyway ;-) that's what I thought :-) -- ir. Kris Verbeeck Development Engineer Ubizen - Ubicenter - Philipssite 5 - 3001 Leuven - Belgium T: +32 16 28 70 64 F: +32 16 28 70 77 Ubizen - We Secure e-business - www.ubizen.com
