Re: Any progress on PR41230 (HEAD issues on cached items)?
On Fri, 18 May 2007, Justin Erenkrantz wrote:
On 5/17/07, Niklas Edmundsson [EMAIL PROTECTED] wrote:
Has there been any progress on PR41230? I submitted a patch that at
least seems to improve the situation that now seems to have seen some
testing by others as well.
As I have stated before, it would be really nice if a fix for this
could be committed, be it my patch or some other solution.
I've committed a variant of this patch to trunk in r539620. Thanks!
Great!
Now it just needs to be included in 2.2.x and I'll be even more happy
:)
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | [EMAIL PROTECTED]
---
Old mufflers never die, they get exhausted.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Re: Any progress on PR41230 (HEAD issues on cached items)?
On May 18, 2007, at 5:26 PM, Justin Erenkrantz wrote:
On 5/18/07, Ruediger Pluem [EMAIL PROTECTED] wrote:
Well, because rv == !OK, wouldn't the CACHE_REMOVE_URL filter hit?
That should do the dirty deed, no? -- justin
No, as the CACHE_REMOVE_URL filter will only work if there is a
cache-handle or a cache-stale_handle. We have neither, as cache-
stale_handle
is set to NULL in the case that the cached entity is *really*
stale and we do
not create a new entity (and thus a cache-handle) in the HEAD case.
Well, let's not clear cache-stale_handle then. How does the
following look? -- justin
+1 !
Index: modules/cache/mod_cache.c
===
--- modules/cache/mod_cache.c (revision 539607)
+++ modules/cache/mod_cache.c (working copy)
@@ -477,8 +477,10 @@
reason = No Last-Modified, Etag, or Expires headers;
}
else if (r-header_only) {
-/* HEAD requests */
-reason = HTTP HEAD request;
+/* Forbid HEAD requests unless we have it cached already */
+if (!cache-stale_handle) {
+reason = HTTP HEAD request;
+}
}
else if (!conf-store_nostore
ap_cache_liststr(NULL, cc_out, no-store, NULL)) {
@@ -596,7 +598,12 @@
* the headers).
*/
-/* Did we have a stale cache entry that really is stale? */
+/* Did we have a stale cache entry that really is stale?
+ *
+ * Note that for HEAD requests, we won't get the body, so for
a stale
+ * HEAD request, we don't remove the entity - instead we let the
+ * CACHE_REMOVE_URL filter remove the stale item from the cache.
+ */
if (cache-stale_handle) {
if (r-status == HTTP_NOT_MODIFIED) {
/* Oh, hey. It isn't that stale! Yay! */
@@ -604,7 +611,7 @@
info = cache-handle-cache_obj-info;
rv = OK;
}
-else {
+else if (!r-header_only) {
/* Oh, well. Toss it. */
cache-provider-remove_entity(cache-stale_handle);
/* Treat the request as if it wasn't conditional. */
@@ -612,8 +619,8 @@
}
}
-/* no cache handle, create a new entity */
-if (!cache-handle) {
+/* no cache handle, create a new entity only for non-HEAD
requests */
+if (!cache-handle !r-header_only) {
rv = cache_create_entity(r, size);
info = apr_pcalloc(r-pool, sizeof(cache_info));
/* We only set info-status upon the initial creation. */
Re: Any progress on PR41230 (HEAD issues on cached items)?
On 05/18/2007 02:23 AM, Justin Erenkrantz wrote: On 5/17/07, Ruediger Pluem [EMAIL PROTECTED] wrote: why. Also the entity is not physically removed from the cache if it is really stale. This does not matter in the non HEAD case as it gets overwritten by the fresh response, but in the HEAD case it should be physically removed IMO. Well, because rv == !OK, wouldn't the CACHE_REMOVE_URL filter hit? That should do the dirty deed, no? -- justin No, as the CACHE_REMOVE_URL filter will only work if there is a cache-handle or a cache-stale_handle. We have neither, as cache-stale_handle is set to NULL in the case that the cached entity is *really* stale and we do not create a new entity (and thus a cache-handle) in the HEAD case. Regards Rüdiger
Re: Any progress on PR41230 (HEAD issues on cached items)?
On 5/18/07, Ruediger Pluem [EMAIL PROTECTED] wrote:
Well, because rv == !OK, wouldn't the CACHE_REMOVE_URL filter hit?
That should do the dirty deed, no? -- justin
No, as the CACHE_REMOVE_URL filter will only work if there is a
cache-handle or a cache-stale_handle. We have neither, as cache-stale_handle
is set to NULL in the case that the cached entity is *really* stale and we do
not create a new entity (and thus a cache-handle) in the HEAD case.
Well, let's not clear cache-stale_handle then. How does the
following look? -- justin
Index: modules/cache/mod_cache.c
===
--- modules/cache/mod_cache.c (revision 539607)
+++ modules/cache/mod_cache.c (working copy)
@@ -477,8 +477,10 @@
reason = No Last-Modified, Etag, or Expires headers;
}
else if (r-header_only) {
-/* HEAD requests */
-reason = HTTP HEAD request;
+/* Forbid HEAD requests unless we have it cached already */
+if (!cache-stale_handle) {
+reason = HTTP HEAD request;
+}
}
else if (!conf-store_nostore
ap_cache_liststr(NULL, cc_out, no-store, NULL)) {
@@ -596,7 +598,12 @@
* the headers).
*/
-/* Did we have a stale cache entry that really is stale? */
+/* Did we have a stale cache entry that really is stale?
+ *
+ * Note that for HEAD requests, we won't get the body, so for a stale
+ * HEAD request, we don't remove the entity - instead we let the
+ * CACHE_REMOVE_URL filter remove the stale item from the cache.
+ */
if (cache-stale_handle) {
if (r-status == HTTP_NOT_MODIFIED) {
/* Oh, hey. It isn't that stale! Yay! */
@@ -604,7 +611,7 @@
info = cache-handle-cache_obj-info;
rv = OK;
}
-else {
+else if (!r-header_only) {
/* Oh, well. Toss it. */
cache-provider-remove_entity(cache-stale_handle);
/* Treat the request as if it wasn't conditional. */
@@ -612,8 +619,8 @@
}
}
-/* no cache handle, create a new entity */
-if (!cache-handle) {
+/* no cache handle, create a new entity only for non-HEAD requests */
+if (!cache-handle !r-header_only) {
rv = cache_create_entity(r, size);
info = apr_pcalloc(r-pool, sizeof(cache_info));
/* We only set info-status upon the initial creation. */
Re: Any progress on PR41230 (HEAD issues on cached items)?
On 05/18/2007 11:26 PM, Justin Erenkrantz wrote:
On 5/18/07, Ruediger Pluem [EMAIL PROTECTED] wrote:
Well, because rv == !OK, wouldn't the CACHE_REMOVE_URL filter hit?
That should do the dirty deed, no? -- justin
No, as the CACHE_REMOVE_URL filter will only work if there is a
cache-handle or a cache-stale_handle. We have neither, as
cache-stale_handle
is set to NULL in the case that the cached entity is *really* stale
and we do
not create a new entity (and thus a cache-handle) in the HEAD case.
Well, let's not clear cache-stale_handle then. How does the
following look? -- justin
Good :-). Thanks.
Index: modules/cache/mod_cache.c
===
--- modules/cache/mod_cache.c(revision 539607)
+++ modules/cache/mod_cache.c(working copy)
@@ -477,8 +477,10 @@
reason = No Last-Modified, Etag, or Expires headers;
}
else if (r-header_only) {
-/* HEAD requests */
-reason = HTTP HEAD request;
+/* Forbid HEAD requests unless we have it cached already */
+if (!cache-stale_handle) {
+reason = HTTP HEAD request;
+}
}
else if (!conf-store_nostore
ap_cache_liststr(NULL, cc_out, no-store, NULL)) {
Is this correct? In the case that we have a HEAD request on a stale cached
entity we would ignore any of the following conditions like
Cache-Control: nostore, Cache-Control: private, an Authorization header in the
request, Vary *, or r-no_cache being set and update the headers of the
entity with the contents of the HEAD response. This sounds wrong to me.
Why not using
(r-header_only !cache-stale_handle)
instead?
Regards
Rüdiger
Re: Any progress on PR41230 (HEAD issues on cached items)?
On 5/18/07, Ruediger Pluem [EMAIL PROTECTED] wrote:
@@ -477,8 +477,10 @@
reason = No Last-Modified, Etag, or Expires headers;
}
else if (r-header_only) {
-/* HEAD requests */
-reason = HTTP HEAD request;
+/* Forbid HEAD requests unless we have it cached already */
+if (!cache-stale_handle) {
+reason = HTTP HEAD request;
+}
}
else if (!conf-store_nostore
ap_cache_liststr(NULL, cc_out, no-store, NULL)) {
Is this correct? In the case that we have a HEAD request on a stale cached
entity we would ignore any of the following conditions like
Cache-Control: nostore, Cache-Control: private, an Authorization header in the
request, Vary *, or r-no_cache being set and update the headers of the
entity with the contents of the HEAD response. This sounds wrong to me.
Why not using
(r-header_only !cache-stale_handle)
instead?
Yah - I was trying to be too cute, I guess. I'll commit to trunk with
it as one if statement and that should resolve this last concern...
Thanks! -- justin
Re: Any progress on PR41230 (HEAD issues on cached items)?
On 5/17/07, Niklas Edmundsson [EMAIL PROTECTED] wrote: Has there been any progress on PR41230? I submitted a patch that at least seems to improve the situation that now seems to have seen some testing by others as well. As I have stated before, it would be really nice if a fix for this could be committed, be it my patch or some other solution. I've committed a variant of this patch to trunk in r539620. Thanks! -- justin
Any progress on PR41230 (HEAD issues on cached items)?
Has there been any progress on PR41230? I submitted a patch that at
least seems to improve the situation that now seems to have seen some
testing by others as well.
As I have stated before, it would be really nice if a fix for this
could be committed, be it my patch or some other solution.
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | [EMAIL PROTECTED]
---
Don't hide your contempt of the contemptible!
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Re: Any progress on PR41230 (HEAD issues on cached items)?
On 5/17/07, Niklas Edmundsson [EMAIL PROTECTED] wrote:
Has there been any progress on PR41230? I submitted a patch that at
least seems to improve the situation that now seems to have seen some
testing by others as well.
As I have stated before, it would be really nice if a fix for this
could be committed, be it my patch or some other solution.
(It'd be helpful if you posted the patch here instead of forcing us to
go to Bugzilla...)
Anyway, the core of the patch is okay - but what's the deal with the
rv = !OK line? I don't see how or why that's necessary as rv is
already initialized to that.
I'd tweak it to be as follows for readability though. If this still
fixes your problem, I can commit and propose for backport. Thanks!
-- justin
Index: modules/cache/mod_cache.c
===
--- modules/cache/mod_cache.c (revision 539156)
+++ modules/cache/mod_cache.c (working copy)
@@ -476,8 +476,10 @@
reason = No Last-Modified, Etag, or Expires headers;
}
else if (r-header_only) {
-/* HEAD requests */
-reason = HTTP HEAD request;
+/* Forbid HEAD requests unless we have it cached already */
+if (!cache-stale_handle) {
+reason = HTTP HEAD request;
+}
}
else if (!conf-store_nostore
ap_cache_liststr(NULL, cc_out, no-store, NULL)) {
@@ -611,8 +613,8 @@
}
}
-/* no cache handle, create a new entity */
-if (!cache-handle) {
+/* no cache handle, create a new entity only for non-HEAD requests */
+if (!cache-handle !r-header_only) {
rv = cache_create_entity(r, size);
info = apr_pcalloc(r-pool, sizeof(cache_info));
/* We only set info-status upon the initial creation. */
Re: Any progress on PR41230 (HEAD issues on cached items)?
On 05/18/2007 01:26 AM, Justin Erenkrantz wrote: On 5/17/07, Niklas Edmundsson [EMAIL PROTECTED] wrote: Has there been any progress on PR41230? I submitted a patch that at least seems to improve the situation that now seems to have seen some testing by others as well. As I have stated before, it would be really nice if a fix for this could be committed, be it my patch or some other solution. (It'd be helpful if you posted the patch here instead of forcing us to go to Bugzilla...) He has a while ago :-): http://mail-archives.apache.org/mod_mbox/httpd-dev/200704.mbox/[EMAIL PROTECTED] Anyway, the core of the patch is okay - but what's the deal with the rv = !OK line? I don't see how or why that's necessary as rv is already initialized to that. I'd tweak it to be as follows for readability though. If this still fixes your problem, I can commit and propose for backport. Thanks! It (meaning your and his patch) does not fix it in all cases IMO. See http://mail-archives.apache.org/mod_mbox/httpd-dev/200704.mbox/[EMAIL PROTECTED] why. Also the entity is not physically removed from the cache if it is really stale. This does not matter in the non HEAD case as it gets overwritten by the fresh response, but in the HEAD case it should be physically removed IMO. Anyway, thanks for removing this rv = !OK stuff. Regards Rüdiger
Re: Any progress on PR41230 (HEAD issues on cached items)?
On 5/17/07, Ruediger Pluem [EMAIL PROTECTED] wrote: why. Also the entity is not physically removed from the cache if it is really stale. This does not matter in the non HEAD case as it gets overwritten by the fresh response, but in the HEAD case it should be physically removed IMO. Well, because rv == !OK, wouldn't the CACHE_REMOVE_URL filter hit? That should do the dirty deed, no? -- justin
