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 95 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_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,
                               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 84 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),
       synchronous_entry_in_use_by_worker_(false),
       has_been_doomed_(false) {
-  DCHECK(synchronous_entry);
+  SetSynchronousData();
 }
 
 SimpleEntryImpl::~SimpleEntryImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!synchronous_entry_) << "synchronous_entry_ = " << synchronous_entry_;
 }
 
 // static
 void SimpleEntryImpl::CreationOperationComplete(
     const CompletionCallback& completion_callback,
@@ skipping 17 matching lines @@
 void SimpleEntryImpl::EntryOperationComplete(
     const CompletionCallback& completion_callback,
     base::WeakPtr<SimpleEntryImpl> entry,
     SimpleSynchronousEntry* sync_entry,
     int result) {
   DCHECK(sync_entry);
   if (entry) {
     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