diff options
author | rvargas@google.com <rvargas@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-08-20 21:18:29 +0000 |
---|---|---|
committer | rvargas@google.com <rvargas@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-08-20 21:18:29 +0000 |
commit | a79837898c13790be9b7094e16b98a01288b4687 (patch) | |
tree | 41eac04be02116e85e657a4c03cf69a1e3c58163 /net/http | |
parent | c2cb8549159ac80ce1d587445513fae5942e568d (diff) | |
download | chromium_src-a79837898c13790be9b7094e16b98a01288b4687.zip chromium_src-a79837898c13790be9b7094e16b98a01288b4687.tar.gz chromium_src-a79837898c13790be9b7094e16b98a01288b4687.tar.bz2 |
Http cache: Fix the code that handles 206s when revalidating
a range from the cache.
BUG=12258
TEST=unittests
Review URL: http://codereview.chromium.org/174039
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@23881 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net/http')
-rw-r--r-- | net/http/http_cache.cc | 64 | ||||
-rw-r--r-- | net/http/http_cache_unittest.cc | 46 | ||||
-rw-r--r-- | net/http/partial_data.cc | 3 |
3 files changed, 91 insertions, 22 deletions
diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc index 23c462e..4de8080 100644 --- a/net/http/http_cache.cc +++ b/net/http/http_cache.cc @@ -1128,6 +1128,23 @@ bool HttpCache::Transaction::ConditionalizeRequest() { return true; } +// We just received some headers from the server. We may have asked for a range, +// in which case partial_ has an object. This could be the first network request +// we make to fulfill the original request, or we may be already reading (from +// the net and / or the cache). If we are not expecting a certain response, we +// just bypass the cache for this request (but again, maybe we are reading), and +// delete partial_ (so we are not able to "fix" the headers that we return to +// the user). This results in either a weird response for the caller (we don't +// expect it after all), or maybe a range that was not exactly what it was asked +// for. +// +// For example, if the original request is for 30KB and we have the last 20KB, +// we ask the server for the first 10KB. If the resourse has changed, we'll +// end up forwarding the 200 back to the user (so far so good). However, if +// we have instead the first 10KB, we end up sending back a byte range response +// for the first 10KB, because we never asked the server for the last part. It's +// just too complicated to restart the whole request from this point; and of +// course, maybe we already returned the headers. bool HttpCache::Transaction::ValidatePartialResponse( const HttpResponseHeaders* headers) { int response_code = headers->response_code(); @@ -1150,30 +1167,38 @@ bool HttpCache::Transaction::ValidatePartialResponse( return false; } - // TODO(rvargas): Handle 206 when the current range is already cached. - bool failure = false; - if (!partial_content) { - if (!partial_.get()) - return false; + if (!partial_.get()) { + // We are not expecting 206 but we may have one. + if (partial_content) + IgnoreRangeRequest(); - // TODO(rvargas): Do we need to consider other results here?. - if (response_code == 200 || response_code == 416) - failure = true; + return false; + } - if (!reading_ && failure) { - // We are expecting 206 or 304 because we asked for a range. Given that - // the server is refusing the request we'll remove the sparse entry. - DoomPartialEntry(true); - mode_ = NONE; - return false; - } + // TODO(rvargas): Do we need to consider other results here?. + bool failure = response_code == 200 || response_code == 416; + + if (partial_->IsCurrentRangeCached()) { + // We asked for "If-None-Match: " so a 206 means a new object. + if (partial_content) + failure = true; if (response_code == 304 && partial_->ResponseHeadersOK(headers)) return false; + } else { + // We asked for "If-Range: " so a 206 means just another range. + if (partial_content && partial_->ResponseHeadersOK(headers)) + return true; + + // 304 is not expected here, but we'll spare the entry. } - if (!failure && partial_.get() && partial_->ResponseHeadersOK(headers)) - return true; + if (failure) { + // We cannot truncate this entry, it has to be deleted. + DoomPartialEntry(true); + mode_ = NONE; + return false; + } IgnoreRangeRequest(); return false; @@ -1352,7 +1377,10 @@ void HttpCache::Transaction::OnNetworkInfoAvailable(int result) { if (response_.headers->HasHeaderValue("cache-control", "no-store")) { cache_->DoomEntry(cache_key_); } else { - WriteResponseInfoToEntry(); + // If we are already reading, we already updated the headers for + // this request; doing it again will change Content-Length. + if (!reading_) + WriteResponseInfoToEntry(); } if (mode_ == UPDATE) { diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc index 96118d8..14299b0 100644 --- a/net/http/http_cache_unittest.cc +++ b/net/http/http_cache_unittest.cc @@ -465,13 +465,19 @@ class RangeTransactionServer { public: RangeTransactionServer() { not_modified_ = false; + modified_ = false; } ~RangeTransactionServer() { not_modified_ = false; + modified_ = false; } + // Returns only 416 or 304 when set. void set_not_modified(bool value) { not_modified_ = value; } + // Returns 206 when revalidating a range (instead of 304). + void set_modified(bool value) { modified_ = value; } + static void RangeHandler(const net::HttpRequestInfo* request, std::string* response_status, std::string* response_headers, @@ -479,9 +485,11 @@ class RangeTransactionServer { private: static bool not_modified_; + static bool modified_; DISALLOW_COPY_AND_ASSIGN(RangeTransactionServer); }; bool RangeTransactionServer::not_modified_ = false; +bool RangeTransactionServer::modified_ = false; // Static. void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request, @@ -519,7 +527,8 @@ void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request, start, end); response_headers->append(content_range); - if (request->extra_headers.find("If-None-Match") == std::string::npos) { + if (request->extra_headers.find("If-None-Match") == std::string::npos || + modified_) { EXPECT_EQ(9, (end - start) % 10); std::string data; for (int block_start = start; block_start < end; block_start += 10) @@ -1766,6 +1775,41 @@ TEST(HttpCache, DISABLED_RangeGET_304) { RemoveMockTransaction(&kRangeGET_TransactionOK); } +// Tests that we deal with 206s when revalidating range requests. +TEST(HttpCache, DISABLED_RangeGET_ModifiedResult) { + MockHttpCache cache; + AddMockTransaction(&kRangeGET_TransactionOK); + std::string headers; + + // Write to the cache (40-49). + RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_TransactionOK, + &headers); + + EXPECT_TRUE(Verify206Response(headers, 40, 49)); + EXPECT_EQ(1, cache.network_layer()->transaction_count()); + EXPECT_EQ(0, cache.disk_cache()->open_count()); + EXPECT_EQ(1, cache.disk_cache()->create_count()); + + // Attempt to read from the cache (40-49). + RangeTransactionServer handler; + handler.set_modified(true); + RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_TransactionOK, + &headers); + + EXPECT_TRUE(Verify206Response(headers, 40, 49)); + EXPECT_EQ(2, cache.network_layer()->transaction_count()); + EXPECT_EQ(1, cache.disk_cache()->open_count()); + EXPECT_EQ(1, cache.disk_cache()->create_count()); + + // And the entry should be gone. + RunTransactionTest(cache.http_cache(), kRangeGET_TransactionOK); + EXPECT_EQ(3, cache.network_layer()->transaction_count()); + EXPECT_EQ(1, cache.disk_cache()->open_count()); + EXPECT_EQ(2, cache.disk_cache()->create_count()); + + RemoveMockTransaction(&kRangeGET_TransactionOK); +} + // Tests that we can cache range requests when the start or end is unknown. // We start with one suffix request, followed by a request from a given point. TEST(HttpCache, DISABLED_UnknownRangeGET_1) { diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc index d24d6fa..70d7d18 100644 --- a/net/http/partial_data.cc +++ b/net/http/partial_data.cc @@ -258,9 +258,6 @@ void PartialData::OnCacheReadCompleted(int result) { current_range_start_ += result; cached_min_len_ -= result; DCHECK(cached_min_len_ >= 0); - } else if (!result) { - // TODO(rvargas): we can detect this error and make sure that we are not - // in a loop of failure/retry. } } |