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 5cf4a7e4e7b1fe24fc72a902dcdd4930203c17e9..798cab75d14c9cfe504c8d2a10846cc31c1c5f4d 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -113,32 +113,17 @@ std::string SimpleEntryImpl::GetKey() const {
 
 Time SimpleEntryImpl::GetLastUsed() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED() << ": Synchronous operations overlapping an asynchronous "
-                     << "operation.";
-    NOTREACHED();
-  }
-  return synchronous_entry_->last_used();
+  return last_used_;
 }
 
 Time SimpleEntryImpl::GetLastModified() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (synchronous_entry_in_use_by_worker_) {
-    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_in_use_by_worker_) {
-    NOTIMPLEMENTED() << ": Synchronous operations overlapping an asynchronous "
-                     << "operation.";
-    NOTREACHED();
-  }
-  return synchronous_entry_->data_size(index);
+  return data_size_[index];
 }
 
 int SimpleEntryImpl::ReadData(int index,
@@ -243,11 +228,12 @@ 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),
       synchronous_entry_in_use_by_worker_(false),
       has_been_doomed_(false) {
-  DCHECK(synchronous_entry);
+  SetSynchronousData();
 }
 
 SimpleEntryImpl::~SimpleEntryImpl() {
@@ -285,8 +271,25 @@ void SimpleEntryImpl::EntryOperationComplete(
     DCHECK(entry->synchronous_entry_in_use_by_worker_);
     DCHECK_EQ(entry->synchronous_entry_, sync_entry);
     entry->synchronous_entry_in_use_by_worker_ = false;
+    entry->SetSynchronousData();
+
   }
   completion_callback.Run(result);
 }
 
+void SimpleEntryImpl::SetSynchronousData() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!synchronous_entry_in_use_by_worker_);
+
+  // TODO(felipeg): These copies to avoid data races are not optimal. While
+  // adding an IO thread index (for fast misses etc...), we can store this data
+  // in that structure. This also solves problems with last_used() on ext4
+  // filesystems not being accurate.

[gavinp, 2013/02/12 16:17:49]

I am, like pasko, afraid of data races, and that's why I wrote the code to do
copies.

I recognize your points about benign data races; even on late loading platforms
like ARM, by the time it really matters (at callback time), we've passed a
memory barrier and we're fine.

But isn't this early to worry  about this? Most of this data properly belongs in
the index structure, which lives on the IO thread. And you're implementing that,
right? So I wrote a big ol' TODO() for you here.

Ricardo, what do you think? I know in the upstream review (add simple cache
backend), you asked me about the duplication of key_, which was being done for
the same reason (and that was an even more benign race).

[felipeg, 2013/02/12 16:23:08]

You are right.
We will need to store this on the index anyway for quick miss operations.

For now, the TODO suffice.

+
+  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