Make synchronous methods on disk_cache::SimpleEntryImpl() reentrant.

net/disk_cache/simple/simple_entry_impl.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/disk_cache/simple/simple_entry_impl.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/location.h"
@@ skipping 193 matching lines @@
   delete this;
 }
 
 std::string SimpleEntryImpl::GetKey() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return key_;
 }
 
 Time SimpleEntryImpl::GetLastUsed() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!synchronous_entry_) {
-    NOTIMPLEMENTED() << ": Synchronous operations overlapping an asynchronous "
-                     << "operation.";
-    NOTREACHED();
-  }
-  return synchronous_entry_->last_used();

[felipeg, 2013/02/12 14:47:48]

If we never set synchronous_entry_ to NULL as my comment in the other CL (by
adding a boolean IsOperationPending() instead of setting the pointer to NULL).

In that case, we can just call synchronous_entry_->last_used().
And this method must be thread safe by either using a lock
Or by just guaranteeing that the returned value will be either the new value or
the old value, but never a corrupt value, and letting the race happen.
It is ok for this kind of getter...

[pasko-google - do not use, 2013/02/12 15:07:12]

I disagree. Data races are never a good excuse for any design. Sorry.

Frankly speaking, I have done some designs where data races were "benign", i.e.
in worst case produced only some incorrectly gathered statistics, and even this
sometimes wakes me up as a nightmare. Here we make eviction decisions based on
this data, which makes the matter very complex and hardly reproducible if a data
race resulted in a very unusual code path.

Also, data races will be reported by data race detection tools, which we would
need to suppress using annotations in our code, which is very complicated and
requires a lot of expertise.

In the end, manual data privatization to threads is a commonly used pattern for
optimization. We may incapsulate it into a single class that would be easier to
update. Occasional inconsistencies when updating code may happen, of course, but
at least it would be buried within the same small class.

+  return last_used_;
 }
 
 Time SimpleEntryImpl::GetLastModified() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!synchronous_entry_) {
-    NOTIMPLEMENTED() << ": Synchronous operations overlapping an asynchronous "
-                     << "operation.";
-    NOTREACHED();
-  }
-  return synchronous_entry_->last_modified();
+  return last_modified_;
 }
 
 int32 SimpleEntryImpl::GetDataSize(int index) const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!synchronous_entry_) {
-    NOTIMPLEMENTED() << ": Synchronous operations overlapping an asynchronous "
-                     << "operation.";
-    NOTREACHED();
-  }
-  return synchronous_entry_->data_size(index);
+  return data_size_[index];
 }
 
 int SimpleEntryImpl::ReadData(int index,
                               int offset,
                               net::IOBuffer* buf,
                               int buf_len,
                               const CompletionCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   // TODO(gavinp): Add support for overlapping reads. The net::HttpCache does
   // make overlapping read requests when multiple transactions access the same
@@ skipping 80 matching lines @@
 int SimpleEntryImpl::ReadyForSparseIO(const CompletionCallback& callback) {
   // TODO(gavinp): Determine if the simple backend should support sparse data.
   DCHECK(thread_checker_.CalledOnValidThread());
   NOTIMPLEMENTED();
   return net::ERR_FAILED;
 }
 
 SimpleEntryImpl::SimpleEntryImpl(
     SimpleSynchronousEntry* synchronous_entry)
     : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
+      path_(synchronous_entry->path()),
       key_(synchronous_entry->key()),
-      synchronous_entry_(synchronous_entry),
+      synchronous_entry_(NULL),
       has_been_doomed_(false) {
-  DCHECK(synchronous_entry);
+  SetSynchronousEntry(synchronous_entry);
 }
 
 SimpleEntryImpl::~SimpleEntryImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!synchronous_entry_) << "synchronous_entry_ = " << synchronous_entry_;
 }
 
 SimpleSynchronousEntry* SimpleEntryImpl::ReleaseSynchronousEntry() {
   DCHECK(synchronous_entry_);
   SimpleSynchronousEntry* retval = synchronous_entry_;
   synchronous_entry_ = NULL;
   return retval;
 }
 
 void SimpleEntryImpl::SetSynchronousEntry(
     SimpleSynchronousEntry* synchronous_entry) {
   DCHECK(!synchronous_entry_) << "synchronous_entry_ = " << synchronous_entry_;
   synchronous_entry_ = synchronous_entry;
+  last_used_ = synchronous_entry_->last_used();
+  last_modified_ = synchronous_entry_->last_modified();
+  for (int i = 0; i < kSimpleEntryFileCount; ++i)
+    data_size_[i] = synchronous_entry_->data_size(i);
 }
 
 }  // namespace disk_cache