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

net/disk_cache/simple/simple_entry_impl.cc

Index: net/disk_cache/simple/simple_entry_impl.cc
diff --git a/net/disk_cache/simple/simple_entry_impl.cc b/net/disk_cache/simple/simple_entry_impl.cc
index 0639b942f6e17f8d5db7cdeb2a67f8070a959a70..b2757517596812390a0ffdbc733108f0d606df07 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -211,32 +211,17 @@ std::string SimpleEntryImpl::GetKey() const {
 
 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,
@@ -337,10 +322,11 @@ int SimpleEntryImpl::ReadyForSparseIO(const CompletionCallback& callback) {
 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() {
@@ -359,6 +345,10 @@ 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