Index: net/http/http_cache_transaction.cc |
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc |
index d5cb94d7909b233e49bbad53cf70e3806372dccc..b37029f076f803247f2a8c0250421b663dcef9f7 100644 |
--- a/net/http/http_cache_transaction.cc |
+++ b/net/http/http_cache_transaction.cc |
@@ -266,6 +266,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache) |
done_reading_(false), |
vary_mismatch_(false), |
couldnt_conditionalize_request_(false), |
+ stopped_caching_(false), |
bypass_lock_for_test_(false), |
fail_conditionalization_for_test_(false), |
io_buf_len_(0), |
@@ -300,7 +301,8 @@ HttpCache::Transaction::~Transaction() { |
} |
} |
- cache_->DoneWithEntry(entry_, this, cancel_request); |
+ ReleaseCacheEntry(cancel_request ? CACHE_ENTRY_PRESERVE |
+ : CACHE_ENTRY_DOOM); |
} else if (cache_pending_) { |
cache_->RemovePendingTransaction(this); |
} |
@@ -323,26 +325,6 @@ int HttpCache::Transaction::WriteMetadata(IOBuffer* buf, int buf_len, |
callback, true); |
} |
-bool HttpCache::Transaction::AddTruncatedFlag() { |
- DCHECK(mode_ & WRITE || mode_ == NONE); |
- |
- // Don't set the flag for sparse entries. |
- if (partial_ && !truncated_) |
- return true; |
- |
- if (!CanResume(true)) |
- return false; |
- |
- // We may have received the whole resource already. |
- if (done_reading_) |
- return true; |
- |
- truncated_ = true; |
- next_state_ = STATE_CACHE_WRITE_TRUNCATED_RESPONSE; |
- DoLoop(OK); |
- return true; |
-} |
- |
LoadState HttpCache::Transaction::GetWriterLoadState() const { |
if (network_trans_.get()) |
return network_trans_->GetLoadState(); |
@@ -457,7 +439,6 @@ int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len, |
DCHECK(buf); |
DCHECK_GT(buf_len, 0); |
DCHECK(!callback.is_null()); |
- |
DCHECK(callback_.is_null()); |
if (!cache_.get()) |
@@ -476,7 +457,10 @@ int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len, |
reading_ = true; |
read_buf_ = buf; |
io_buf_len_ = buf_len; |
- if (network_trans_) { |
+ if (stopped_caching_ && (mode_ & WRITE) && |
rvargas (doing something else)
2015/08/19 23:46:38
Why do we need to check the mode here? (how can we
asanka
2015/09/04 19:09:04
I removed the mode_ check from StopCaching() and w
rvargas (doing something else)
2015/09/11 23:56:17
hmmm. Related to comments elsewhere.
|
+ !(effective_load_flags_ & LOAD_PREFERRING_CACHE) && entry_) { |
rvargas (doing something else)
2015/08/19 23:46:38
why do we look at load_flags_?
asanka
2015/09/04 19:09:03
Originally did so because LOAD_PREFERRING_CACHE ma
|
+ next_state_ = STATE_RELEASE_CACHE_ENTRY_AND_CONTINUE_USING_NETWORK; |
+ } else if (network_trans_) { |
DCHECK(mode_ == WRITE || mode_ == NONE || |
(mode_ == READ_WRITE && partial_)); |
next_state_ = STATE_NETWORK_READ; |
@@ -495,20 +479,16 @@ int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len, |
} |
void HttpCache::Transaction::StopCaching() { |
- // We really don't know where we are now. Hopefully there is no operation in |
- // progress, but nothing really prevents this method to be called after we |
- // returned ERR_IO_PENDING. We cannot attempt to truncate the entry at this |
- // point because we need the state machine for that (and even if we are really |
- // free, that would be an asynchronous operation). In other words, keep the |
- // entry how it is (it will be marked as truncated at destruction), and let |
- // the next piece of code that executes know that we are now reading directly |
- // from the net. |
- // TODO(mmenke): This doesn't release the lock on the cache entry, so a |
- // future request for the resource will be blocked on this one. |
- // Fix this. |
- if (cache_.get() && entry_ && (mode_ & WRITE) && network_trans_.get() && |
- !is_sparse_ && !range_requested_) { |
- mode_ = NONE; |
+ DCHECK(!(effective_load_flags_ & LOAD_PREFETCH)); |
rvargas (doing something else)
2015/08/19 23:46:38
isn't this outside of the control of this code? Wh
asanka
2015/09/04 19:09:04
Not sure how this crept in. Removed.
|
+ DCHECK(callback_.is_null()); |
+ |
+ // StopCaching() only affects cache writes and is meant to prevent excessively |
+ // large resources from clogging up the cache. If this entry is being read |
+ // exclusively from the cache already, then we are going to ignore the |
rvargas (doing something else)
2015/08/19 23:46:38
I'd probably remove the second sentence, as it is
asanka
2015/09/04 19:09:03
Removed the check and comment since we just set th
|
+ // StopCaching() request. |
+ if (mode_ & WRITE) { |
+ stopped_caching_ = true; |
+ UpdateTransactionPattern(PATTERN_NOT_COVERED); |
} |
} |
@@ -529,17 +509,13 @@ int64 HttpCache::Transaction::GetTotalReceivedBytes() const { |
} |
void HttpCache::Transaction::DoneReading() { |
- if (cache_.get() && entry_) { |
rvargas (doing something else)
2015/08/19 23:46:38
We still need this code, otherwise it will crash a
asanka
2015/09/04 19:09:03
Done.
|
- DCHECK_NE(mode_, UPDATE); |
- if (mode_ & WRITE) { |
- DoneWritingToEntry(true); |
- } else if (mode_ & READ) { |
- // It is necessary to check mode_ & READ because it is possible |
rvargas (doing something else)
2015/08/19 23:46:38
We should be able to fix this code now and make th
asanka
2015/09/04 19:09:03
Done.
|
- // for mode_ to be NONE and entry_ non-NULL with a write entry |
- // if StopCaching was called. |
- cache_->DoneReadingFromEntry(entry_, this); |
- entry_ = NULL; |
- } |
+ DCHECK_NE(mode_, UPDATE); |
+ |
+ if (mode_ & WRITE) { |
+ DoneWritingToEntry(true); |
+ } else if (mode_ & READ) { |
+ cache_->DoneReadingFromEntry(entry_, this); |
+ entry_ = nullptr; |
} |
} |
@@ -902,6 +878,9 @@ int HttpCache::Transaction::DoLoop(int result) { |
case STATE_CACHE_READ_METADATA_COMPLETE: |
rv = DoCacheReadMetadataComplete(rv); |
break; |
+ case STATE_RELEASE_CACHE_ENTRY_AND_CONTINUE_USING_NETWORK: |
+ rv = DoReleaseCacheEntryAndContinueUsingNetwork(); |
+ break; |
case STATE_NETWORK_READ: |
DCHECK_EQ(OK, rv); |
rv = DoNetworkRead(); |
@@ -1411,8 +1390,8 @@ int HttpCache::Transaction::DoSendRequestComplete(int result) { |
// If we tried to conditionalize the request and failed, we know |
// we won't be reading from the cache after this point. |
- if (couldnt_conditionalize_request_) |
- mode_ = WRITE; |
+ if (couldnt_conditionalize_request_ && mode_ == READ_WRITE) |
+ mode_ = stopped_caching_ ? NONE : WRITE; |
rvargas (doing something else)
2015/08/19 23:46:38
If we switch to NONE, shouldn't we release the cac
asanka
2015/09/04 19:09:03
Removed this logic.
|
if (result == OK) { |
next_state_ = STATE_SUCCESSFUL_SEND_REQUEST; |
@@ -1476,6 +1455,16 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() { |
new_response_ = new_response; |
if (!ValidatePartialResponse() && !auth_response_.headers.get()) { |
+ if (reading_) { |
rvargas (doing something else)
2015/08/19 23:46:38
Is this block related to this change? can we make
asanka
2015/09/04 19:09:03
Removed.
|
+ // This transaction has already returned headers and possibly response |
+ // data to the client on the assumption that future network requests will |
+ // be validated. If this assumption is false, the transaction can't |
+ // proceed any further. |
+ int rv = ReleaseCacheEntry(CACHE_ENTRY_DOOM); |
+ DCHECK_EQ(rv, OK); |
+ return ERR_UNEXPECTED; |
+ } |
+ |
// Something went wrong with this request and we have to restart it. |
// If we have an authentication response, we are exposed to weird things |
// hapenning if the user cancels the authentication before we receive |
@@ -1489,6 +1478,20 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() { |
return OK; |
} |
+ if (handling_206_ && stopped_caching_) { |
+ // The transaction successfully switched over to streaming exclusively from |
+ // the network. |
rvargas (doing something else)
2015/08/19 23:46:38
why?. Or what do you mean by "exclusively". This m
asanka
2015/09/04 19:09:03
I expanded the condition here to only include the
|
+ DCHECK(partial_ && partial_->IsLastRange() && |
+ !partial_->IsCurrentRangeCached()); |
+ DCHECK(mode_ & WRITE); |
+ int rv = ReleaseCacheEntry(CACHE_ENTRY_PRESERVE_PREEXISTING); |
+ DCHECK_EQ(rv, OK); |
+ mode_ = NONE; |
+ stopped_caching_ = false; |
+ next_state_ = STATE_NETWORK_READ; |
+ return OK; |
+ } |
+ |
if (handling_206_ && mode_ == READ_WRITE && !truncated_ && !is_sparse_) { |
// We have stored the full entry, but it changed and the server is |
// sending a range. We have to delete the old entry. |
@@ -1508,9 +1511,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() { |
int ret = cache_->DoomEntry(cache_key_, NULL); |
DCHECK_EQ(OK, ret); |
} |
- cache_->DoneWritingToEntry(entry_, true); |
- entry_ = NULL; |
- mode_ = NONE; |
+ DoneWritingToEntry(true); |
} |
// Invalidate any cached GET with a successful POST. |
@@ -1529,6 +1530,11 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() { |
return OK; |
} |
+ if (stopped_caching_) { |
+ next_state_ = STATE_PARTIAL_HEADERS_RECEIVED; |
+ return OK; |
+ } |
+ |
// Are we expecting a response to a conditional query? |
if (mode_ == READ_WRITE || mode_ == UPDATE) { |
if (new_response->headers->response_code() == 304 || handling_206_) { |
@@ -1778,8 +1784,6 @@ int HttpCache::Transaction::DoNetworkReadComplete(int result) { |
if (!cache_.get()) |
return ERR_UNEXPECTED; |
- // If there is an error or we aren't saving the data, we are done; just wait |
rvargas (doing something else)
2015/08/19 23:46:38
Why remove this?
asanka
2015/09/04 19:09:04
Collateral. In an intermediate stage I experimente
|
- // until the destructor runs to see if we can keep the data. |
if (mode_ == NONE || result < 0) |
return result; |
@@ -1851,12 +1855,11 @@ int HttpCache::Transaction::DoCacheWriteData(int num_bytes) { |
} |
int HttpCache::Transaction::DoCacheWriteDataComplete(int result) { |
- if (entry_) { |
- if (net_log_.IsCapturing()) { |
- net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_DATA, |
- result); |
- } |
+ if (entry_ && net_log_.IsCapturing()) { |
+ net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_DATA, |
+ result); |
} |
+ |
if (!cache_.get()) |
return ERR_UNEXPECTED; |
@@ -1882,13 +1885,11 @@ int HttpCache::Transaction::DoCacheWriteDataComplete(int result) { |
} |
} |
- if (result == 0) { |
- // End of file. This may be the result of a connection problem so see if we |
- // have to keep the entry around to be flagged as truncated later on. |
- if (done_reading_ || !entry_ || partial_ || |
- response_.headers->GetContentLength() <= 0) { |
- DoneWritingToEntry(true); |
- } |
+ // End of file. This may be the result of a connection problem so see if we |
+ // have to keep the entry around to be flagged as truncated later on. |
rvargas (doing something else)
2015/08/19 23:46:39
nit: "End of file" is not a given anymore (as it i
asanka
2015/09/17 21:59:40
Comment expanded.
|
+ if (result == 0 && (done_reading_ || !entry_ || partial_ || |
+ response_.headers->GetContentLength() <= 0)) { |
+ DoneWritingToEntry(true); |
} |
return result; |
@@ -1900,7 +1901,64 @@ int HttpCache::Transaction::DoCacheWriteTruncatedResponse() { |
} |
int HttpCache::Transaction::DoCacheWriteTruncatedResponseComplete(int result) { |
- return OnWriteResponseInfoToEntryComplete(result); |
+ OnWriteResponseInfoToEntryComplete(result); |
+ if (entry_) { |
rvargas (doing something else)
2015/08/19 23:46:38
Why can we be here without entry_?
asanka
2015/09/04 19:09:04
OnWriteResponseInfoToEntry() will doom and release
|
+ cache_->DoneWritingToEntry(entry_, true); |
+ entry_ = nullptr; |
+ } |
+ return OK; |
+} |
+ |
+int HttpCache::Transaction::DoReleaseCacheEntryAndContinueUsingNetwork() { |
+ DCHECK(entry_); |
+ DCHECK(mode_ & WRITE); |
+ |
+ if (request_->method != "GET") { |
rvargas (doing something else)
2015/08/19 23:46:38
look at this on StopCaching?
asanka
2015/09/04 19:09:03
StopCaching() can be called prior to Start() being
rvargas (doing something else)
2015/09/11 23:56:17
Wait, why do we want to support that? That use cas
asanka
2015/09/17 21:59:40
It was done initially on the assumption that calle
|
+ // Skipping between network and cache reads are only expected to work with |
+ // GET requests. Ignore stopped_caching_ for everything else. |
+ stopped_caching_ = false; |
+ next_state_ = network_trans_ ? STATE_NETWORK_READ : STATE_CACHE_READ_DATA; |
+ return OK; |
+ } |
+ |
+ // The network transaction can only be trivially reused if it can promise to |
+ // deliver all the bits necessary to fulfil the original request. |
+ if (network_trans_ && (!partial_ || partial_->IsLastRange())) { |
+ // Unless this was a sparse entry or a complete preexiting entry, just get |
rvargas (doing something else)
2015/08/19 23:46:39
typo: preexisting
asanka
2015/09/04 19:09:03
comment changed.
|
+ // rid of the cache entry entirely. If the requestor isn't going to use the |
+ // entry, we are going to assume that it's unlikely that anyone will. |
+ int rv = ReleaseCacheEntry(CACHE_ENTRY_PRESERVE_PREEXISTING); |
+ DCHECK_EQ(rv, OK); |
+ |
+ mode_ = NONE; |
rvargas (doing something else)
2015/08/19 23:46:38
Do this inside Realease?
asanka
2015/09/04 19:09:03
Done.
|
+ stopped_caching_ = false; |
rvargas (doing something else)
2015/08/19 23:46:38
Do we have to set this to false (and elsewhere)? I
rvargas (doing something else)
2015/09/11 23:56:17
I think you missed this :)
asanka
2015/09/17 21:59:40
Indeed! Done.
|
+ next_state_ = STATE_NETWORK_READ; |
+ return OK; |
+ } |
+ |
+ // The existing network transaction doesn't cover the entire range we wanted. |
+ // Let's discard the network transaction and create a new one that will cover |
+ // the entire range we need. |
+ if (network_trans_) |
+ ResetNetworkTransaction(); |
+ |
+ // partial_ == nullptr case was handled above. From here we can assume that |
+ // this transaction has a valid partial state. |
+ if (!partial_->SkipCacheForRemainder()) { |
+ // PartialData only refuses to switch to network reads if the entire |
+ // resource has been accounted for. There's nothing more to do. |
+ stopped_caching_ = false; |
+ return OK; |
+ } |
+ |
+ partial_->PrepareCacheValidation(entry_->disk_entry, |
+ &custom_request_->extra_headers); |
+ DCHECK(!partial_->IsCurrentRangeCached()); |
+ |
+ // Since we are supposedly switching to network reads from cache reads, we |
+ // still need to validate the cache entry. Since partial_ is now switched to |
+ // network reads, it will continue with network reads from here on. |
+ return BeginCacheValidation(); |
} |
//----------------------------------------------------------------------------- |
@@ -2153,6 +2211,11 @@ int HttpCache::Transaction::BeginCacheValidation() { |
// know we won't be falling back to using the cache entry in the |
// LOAD_FROM_CACHE_IF_OFFLINE case. |
if (!ConditionalizeRequest()) { |
+ DCHECK(!reading_); // Conditionalization should only depend on the state |
rvargas (doing something else)
2015/08/19 23:46:38
nit:
// comment
// blah
DCHECK();
asanka
2015/09/04 19:09:03
Done.
|
+ // of the cache prior to starting the request and the |
+ // request method. Failure is unexpected after the |
+ // client has started reading the stream. |
+ |
couldnt_conditionalize_request_ = true; |
UpdateTransactionPattern(PATTERN_ENTRY_CANT_CONDITIONALIZE); |
if (partial_) |
@@ -2673,6 +2736,56 @@ int HttpCache::Transaction::OnWriteResponseInfoToEntryComplete(int result) { |
return OK; |
} |
+int HttpCache::Transaction::ReleaseCacheEntry( |
+ CacheEntryDisposition entry_disposition) { |
+ if (!cache_ || !entry_) |
+ return OK; |
+ |
+ if (entry_disposition == CACHE_ENTRY_PRESERVE_PREEXISTING) { |
+ // The CACHE_ENTRY_PRESERVE_PREEXISTING option is used to indicate that the |
+ // current client is not interested in keeping the cache entry around. Hence |
+ // get rid of the cache entry unless it's sparse or it existed in its |
+ // entirety prior to this transaction. |
rvargas (doing something else)
2015/08/19 23:46:38
Why in its entirety?. If there's a truncated entry
asanka
2015/09/04 19:09:03
The deletion is based on the assumption that most
rvargas (doing something else)
2015/09/11 23:56:17
I suspect the assumption is correct, but I also wo
asanka
2015/09/17 21:59:40
True. It's also not a necessary behavior change fo
|
+ entry_disposition = is_sparse_ || (mode_ == READ_WRITE && !truncated_) |
rvargas (doing something else)
2015/08/19 23:46:38
is_sparse || mode & read || truncated_ ?.
asanka
2015/09/17 21:59:40
Code removed.
|
+ ? CACHE_ENTRY_PRESERVE |
+ : CACHE_ENTRY_DOOM; |
+ } |
+ |
+ // Nothing special needs to happen if this transaction didn't write to the |
+ // entry. Just leave it as is and ignore |entry_disposition|. |
+ if (!entry_->writer) { |
rvargas (doing something else)
2015/08/19 23:46:38
We should let the cache decide this part.
asanka
2015/09/04 19:09:03
Done.
|
+ RecordHistograms(); |
+ cache_->DoneReadingFromEntry(entry_, this); |
+ entry_ = nullptr; |
+ return OK; |
+ } |
+ |
+ // TODO(asanka): This shouldn't be necessary. HttpCache::DoneWritingToEntry |
+ // should destroy the entry if it's already doomed instead of relying on the |
+ // |success| parameter to decide to do so. |
+ if (entry_->doomed) |
+ entry_disposition = CACHE_ENTRY_DOOM; |
rvargas (doing something else)
2015/08/19 23:46:38
Does this do anything?
asanka
2015/09/04 19:09:04
DoneWithEntry would only destroy the entry and fir
rvargas (doing something else)
2015/09/11 23:56:17
if entry_->doomed is on, the entry will be discard
asanka
2015/09/17 21:59:40
Yes. But if there are pending transactions, then H
rvargas (doing something else)
2015/09/19 01:09:34
I see what you mean. I agree that the cache should
|
+ |
+ // The entry should be kept around only if we can use it later. However, the |
+ // entry should be marked as truncated if the transaction stopped writing to |
+ // it prematurely. |
+ if (entry_disposition == CACHE_ENTRY_PRESERVE && !is_sparse_ && reading_ && |
rvargas (doing something else)
2015/08/19 23:46:38
is_sparse != partial_ && !truncated_ (which is wha
asanka
2015/09/17 21:59:40
I moved this condition to AbandonCacheEntry().
|
+ !done_reading_ && (mode_ & WRITE)) { |
+ if (CanResume(true)) { |
+ truncated_ = true; |
+ return DoCacheWriteTruncatedResponse(); |
rvargas (doing something else)
2015/08/19 23:46:38
I don't believe we call a SM method directly anywh
asanka
2015/09/04 19:09:04
Yeah. Fixed. I moved the logic for abandoning a ca
|
+ } |
+ // The entry is incomplete and cannot be resumed. The data we have cannot be |
+ // used. |
+ entry_disposition = CACHE_ENTRY_DOOM; |
+ } |
+ |
+ DCHECK_EQ(this, entry_->writer); |
+ cache_->DoneWritingToEntry(entry_, entry_disposition == CACHE_ENTRY_PRESERVE); |
+ entry_ = nullptr; |
+ return OK; |
+} |
+ |
void HttpCache::Transaction::DoneWritingToEntry(bool success) { |
if (!entry_) |
return; |
@@ -2681,6 +2794,7 @@ void HttpCache::Transaction::DoneWritingToEntry(bool success) { |
cache_->DoneWritingToEntry(entry_, success); |
entry_ = NULL; |
+ stopped_caching_ = false; |
mode_ = NONE; // switch to 'pass through' mode |
} |
@@ -2702,9 +2816,8 @@ int HttpCache::Transaction::OnCacheReadError(int result, bool restart) { |
if (restart) { |
DCHECK(!reading_); |
DCHECK(!network_trans_.get()); |
- cache_->DoneWithEntry(entry_, this, false); |
- entry_ = NULL; |
- is_sparse_ = false; |
rvargas (doing something else)
2015/08/19 23:46:38
We're losing this part (and the transaction will b
asanka
2015/09/04 19:09:04
Reinstated.
|
+ int rv = ReleaseCacheEntry(CACHE_ENTRY_DOOM); |
+ DCHECK_EQ(rv, OK); |
partial_.reset(); |
next_state_ = STATE_GET_BACKEND; |
return OK; |
@@ -2729,13 +2842,13 @@ void HttpCache::Transaction::OnAddToEntryTimeout(base::TimeTicks start_time) { |
void HttpCache::Transaction::DoomPartialEntry(bool delete_object) { |
DVLOG(2) << "DoomPartialEntry"; |
int rv = cache_->DoomEntry(cache_key_, NULL); |
- DCHECK_EQ(OK, rv); |
- cache_->DoneWithEntry(entry_, this, false); |
- entry_ = NULL; |
+ DCHECK_EQ(rv, OK); |
+ rv = ReleaseCacheEntry(CACHE_ENTRY_DOOM); |
+ DCHECK_EQ(rv, OK); |
is_sparse_ = false; |
truncated_ = false; |
if (delete_object) |
- partial_.reset(NULL); |
+ partial_.reset(); |
} |
int HttpCache::Transaction::DoPartialNetworkReadCompleted(int result) { |
@@ -2843,7 +2956,8 @@ void HttpCache::Transaction::UpdateTransactionPattern( |
} |
void HttpCache::Transaction::RecordHistograms() { |
- DCHECK_NE(PATTERN_UNDEFINED, transaction_pattern_); |
+ if (transaction_pattern_ == PATTERN_UNDEFINED) |
rvargas (doing something else)
2015/08/19 23:46:38
nit: consolidate with the next line.
rvargas (doing something else)
2015/08/19 23:46:39
Now that we can reach this code without having ins
asanka
2015/09/04 19:09:04
Done. Also consolidated with next line.
|
+ return; |
if (!cache_.get() || !cache_->GetCurrentBackend() || |
cache_->GetCurrentBackend()->GetCacheType() != DISK_CACHE || |
cache_->mode() != NORMAL || request_->method != "GET") { |