Summary: | mod_cache doesn't handle If-Range correctly | ||
---|---|---|---|
Product: | Apache httpd-2 | Reporter: | moog <moog> |
Component: | mod_cache | Assignee: | Apache HTTPD Bugs Mailing List <bugs> |
Status: | RESOLVED FIXED | ||
Severity: | normal | Keywords: | FixedInTrunk, PatchAvailable |
Priority: | P2 | ||
Version: | 2.2.8 | ||
Target Milestone: | --- | ||
Hardware: | PC | ||
OS: | Linux |
Description
moog
2008-03-11 08:16:13 UTC
Can you please provide a request and the response headers of the corresponding response to this request where you get only the specified range of the entity instead of the whole one? After ensuring doc.txt was in the cache and expired, I updated it with "date >doc.txt" then requested a range of it: GET /doc.txt HTTP/1.1 Host: 127.0.0.1 Range: bytes=10-20 If-Range: Tue, 11 Mar 2008 23:57:41 GMT HTTP/1.1 206 Partial Content Date: Wed, 12 Mar 2008 00:04:26 GMT Server: Apache/2.2.8 (Unix) Last-Modified: Wed, 12 Mar 2008 00:03:22 GMT ETag: "310314-1d-448322a4b8680" Accept-Ranges: bytes Content-Length: 11 Content-Range: bytes 10-20/29 Content-Type: text/plain 00:03:22 G Does the following patch fix your problem? Index: modules/cache/mod_cache.c =================================================================== --- modules/cache/mod_cache.c (revision 634179) +++ modules/cache/mod_cache.c (working copy) @@ -613,6 +613,12 @@ cache->provider->remove_entity(cache->stale_handle); /* Treat the request as if it wasn't conditional. */ cache->stale_handle = NULL; + /* + * Restore the original request headers as they may be needed + * by further output filters like the byterange filter to make + * the correct decisions. + */ + r->headers_in = cache->stale_headers; } } Yes, your patch fixes the problem, thanks very much! Committed to trunk as r636386 (http://svn.apache.org/viewvc?view=rev&revision=636386). Sorry, spoke too soon... although your patch fixes the problem when the backend is the local disk, it has no effect when the backend is another server reverse-proxied with mod_proxy. GET /proxied/date.txt HTTP/1.1 Host: 127.0.0.1 Range: bytes=10-20 If-Range: Wed, 12 Mar 2008 18:48:57 GMT causes Apache to make the following request to the backend: GET /store/date.txt HTTP/1.1 Host: [backend] Range: bytes=10-20 If-None-Match: "3e61762-1d-44841e3b1d840" If-Modified-Since: Wed, 12 Mar 2008 18:48:57 GMT X-Forwarded-For: 127.0.0.1 X-Forwarded-Host: 127.0.0.1 X-Forwarded-Server: box Connection: Keep-Alive and since the backend has updated content, it delivers the new content, but only part of it, because that's what it was asked for: HTTP/1.1 206 Partial Content Date: Wed, 12 Mar 2008 18:51:00 GMT Server: Apache/2.2.3 (Debian) mod_ssl/2.2.3 OpenSSL/0.9.8c WebAuth/3.5.3 Last-Modified: Wed, 12 Mar 2008 18:49:59 GMT ETag: "3e61762-1d-44841e763e3c0" Accept-Ranges: bytes Content-Length: 11 Content-Range: bytes 10-20/29 Keep-Alive: timeout=15, max=100 Connection: Keep-Alive Content-Type: text/plain; charset=ISO-8859-1 18:49:59 G Then Apache delivers this partial content back to the client, not respecting the If-Range request: HTTP/1.1 206 Partial Content Date: Wed, 12 Mar 2008 18:51:00 GMT Server: Apache/2.2.3 (Debian) mod_ssl/2.2.3 OpenSSL/0.9.8c WebAuth/3.5.3 Last-Modified: Wed, 12 Mar 2008 18:49:59 GMT ETag: "3e61762-1d-44841e763e3c0" Accept-Ranges: bytes Content-Length: 11 Content-Range: bytes 10-20/29 Content-Type: text/plain; charset=ISO-8859-1 18:49:59 G Perhaps when a client makes an If-Range request, Apache too needs to make an If-Range request to the backend, rather than an If-Modified-Since, so that it will retrieve the full content when it's needed. (In reply to comment #6) > Sorry, spoke too soon... although your patch fixes the problem when the backend > is the local disk, it has no effect when the backend is another server > reverse-proxied with mod_proxy. Can you please check if the following patch fixes your problem: Index: modules/cache/cache_storage.c =================================================================== --- modules/cache/cache_storage.c (Revision 636503) +++ modules/cache/cache_storage.c (Arbeitskopie) @@ -283,9 +283,18 @@ apr_table_unset(r->headers_in, "If-Match"); apr_table_unset(r->headers_in, "If-Modified-Since"); apr_table_unset(r->headers_in, "If-None-Match"); - apr_table_unset(r->headers_in, "If-Range"); apr_table_unset(r->headers_in, "If-Unmodified-Since"); + /* + * If we have an If-Range header in the original request, we + * also need to remove a possible Range header as otherwise + * we might get back a Range response where we shouldn't. + */ + if (apr_table_get(r->headers_in, "If-Range")) { + apr_table_unset(r->headers_in, "Range"); + } + apr_table_unset(r->headers_in, "If-Range"); + etag = apr_table_get(h->resp_hdrs, "ETag"); lastmod = apr_table_get(h->resp_hdrs, "Last-Modified"); Better check the following patch: Index: modules/cache/cache_storage.c =================================================================== --- modules/cache/cache_storage.c (Revision 636503) +++ modules/cache/cache_storage.c (Arbeitskopie) @@ -286,6 +286,12 @@ apr_table_unset(r->headers_in, "If-Range"); apr_table_unset(r->headers_in, "If-Unmodified-Since"); + /* Do not do Range requests with our own conditionals: If + * we get 304 the Range does not matter and otherwise the + * entity changed and we want to have the complete entity + */ + apr_table_unset(r->headers_in, "Range"); + etag = apr_table_get(h->resp_hdrs, "ETag"); lastmod = apr_table_get(h->resp_hdrs, "Last-Modified"); I made up my mind and in case we set our own conditionals for the request to the backend a Range request makes no sense for the reason stated in the comment. > /* Do not do Range requests with our own conditionals: If
> * we get 304 the Range does not matter and otherwise the
> * entity changed and we want to have the complete entity
Good idea. I've applied this third patch, and not the first two, and in all the cases mentioned earlier I now get the correct behaviour. Thanks again.
(In reply to comment #9) > > /* Do not do Range requests with our own conditionals: If > > * we get 304 the Range does not matter and otherwise the > > * entity changed and we want to have the complete entity > > Good idea. I've applied this third patch, and not the first two, and in all > the cases mentioned earlier I now get the correct behaviour. Thanks again. > The first patch is nevertheless correct. I now applied the third one to trunk as well as r636653 (http://svn.apache.org/viewvc?view=rev&revision=636653). Thanks for testing. (In reply to comment #10) > The first patch is nevertheless correct. I now applied the third one to trunk I'm now using both the first and third patches (r636386 and r636653) and I'm getting the correct behaviour. Thanks. Proposed for backport to 2.2.x as r660284 (http://svn.apache.org/viewvc?rev=660284&view=rev). Backported to v2.2 and released in v2.2.9. |