Make SimpleEntryImpl::Close asynchronous.

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 68 matching lines @@
   // underlying files. On POSIX, this is fine; the files are still open on the
   // SimpleSynchronousEntry, and operations can even happen on them. The files
   // will be removed from the filesystem when they are closed.
   DoomEntry(path_, key_, CompletionCallback());
 #else
   NOTIMPLEMENTED();
 #endif
 }
 
 void SimpleEntryImpl::Close() {
-  if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED();
-    delete this;
-    return;
+  if (!synchronous_entry_in_use_by_worker_) {
+    WorkerPool::PostTask(FROM_HERE,
+                         base::Bind(&SimpleSynchronousEntry::Close,
+                                    base::Unretained(synchronous_entry_)),
+                         true);
   }
-  DCHECK(synchronous_entry_);

[Randy Smith (Not in Mondays), 2013/02/26 18:47:58]

I don't see how this change might result in synchronous_entry_ being null at the
time of a SimpleEntryImpl::Close(); it seems like you are arranging for
synchronous_entry_ to potentially outlive, SimpleEntryImpl, not be destroyed
before it.

[gavinp, 2013/02/26 21:00:34]

You're right. This was cruft that got mismerged when an upstream review removed
this DCHECK. Thank you.

[Randy Smith (Not in Mondays), 2013/02/26 21:13:11]

Still a little confused--the last patch set still doesn't have the DCHECK. 
Should it?

[gavinp, 2013/02/27 21:50:01]

Aha, I was also confused. In response to this comment, I removed the DCHECK in
SimpleEntryImpl::~SimpleEntryImpl. The DCHECK that was here in this CL in
earlier revisions isn't needed any more: rebases from upstream changes made it
redundant.

See my reply to your comment about scoped_ptr<> for more on this topic.

-  WorkerPool::PostTask(FROM_HERE,
-                       base::Bind(&SimpleSynchronousEntry::Close,
-                                  base::Unretained(synchronous_entry_)),
-                       true);
-  synchronous_entry_ = NULL;
   // Entry::Close() is expected to release this entry. See disk_cache.h for
   // details.
   delete this;
 }
 
 std::string SimpleEntryImpl::GetKey() const {
   return key_;
 }
 
 Time SimpleEntryImpl::GetLastUsed() const {
   return last_used_;
 }
 
 Time SimpleEntryImpl::GetLastModified() const {
   return last_modified_;
 }
 
 int32 SimpleEntryImpl::GetDataSize(int index) const {
   return data_size_[index];
 }
 
 int SimpleEntryImpl::ReadData(int index,
                               int offset,
                               net::IOBuffer* buf,
                               int buf_len,
                               const CompletionCallback& callback) {
   // TODO(gavinp): Add support for overlapping reads. The net::HttpCache does
   // make overlapping read requests when multiple transactions access the same
-  // entry as read only.
+  // entry as read only. This might make calling SimpleSynchronousEntry::Close()
+  // correctly more tricky (see SimpleEntryImpl::EntryOperationComplete).
   if (synchronous_entry_in_use_by_worker_) {
     NOTIMPLEMENTED();
     CHECK(false);
   }
   synchronous_entry_in_use_by_worker_ = true;
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
-                 callback, weak_ptr_factory_.GetWeakPtr());
+                 callback, weak_ptr_factory_.GetWeakPtr(), synchronous_entry_);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::ReadData,
                                   base::Unretained(synchronous_entry_),
                                   index, offset, make_scoped_refptr(buf),
                                   buf_len, sync_operation_callback),
                        true);
   return net::ERR_IO_PENDING;
 }
 
 int SimpleEntryImpl::WriteData(int index,
                                int offset,
                                net::IOBuffer* buf,
                                int buf_len,
                                const CompletionCallback& callback,
                                bool truncate) {
   if (synchronous_entry_in_use_by_worker_) {
     NOTIMPLEMENTED();
     CHECK(false);
   }
   synchronous_entry_in_use_by_worker_ = true;
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
-                 callback, weak_ptr_factory_.GetWeakPtr());
+                 callback, weak_ptr_factory_.GetWeakPtr(), synchronous_entry_);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::WriteData,
                                   base::Unretained(synchronous_entry_),
                                   index, offset, make_scoped_refptr(buf),
                                   buf_len, sync_operation_callback, truncate),
                        true);
   return net::ERR_IO_PENDING;
 }
 
 int SimpleEntryImpl::ReadSparseData(int64 offset,
@@ skipping 64 matching lines @@
     return;
   }
   *out_entry = new SimpleEntryImpl(sync_entry);
   completion_callback.Run(net::OK);
 }
 
 // static
 void SimpleEntryImpl::EntryOperationComplete(
     const CompletionCallback& completion_callback,
     base::WeakPtr<SimpleEntryImpl> entry,
+    SimpleSynchronousEntry* sync_entry,
     int result) {
   if (entry) {
     DCHECK(entry->synchronous_entry_in_use_by_worker_);
     entry->synchronous_entry_in_use_by_worker_ = false;
     entry->SetSynchronousData();
+  } else {
+    // |entry| must have had Close() called while this operation was in flight.
+    // Since the simple cache now only supports one pending entry operation in
+    // flight at a time, it's safe to now call Close() on |sync_entry|.
+    WorkerPool::PostTask(FROM_HERE,
+                         base::Bind(&SimpleSynchronousEntry::Close,
+                                    base::Unretained(sync_entry)),

[Randy Smith (Not in Mondays), 2013/02/26 18:47:58]

I would feel better about this change if the ownership model for sync_entry was
documented either directly or by references somewhere in this class (probably
the right place is simple_entry_impl.h:synchronous-entry).  Despite the fact of
it having perfectly valid uses, base::Unretained() makes me twitch, and so I
want to make it as easily as possible to verify that the uses are reasonable
ones. 

[gavinp, 2013/02/26 21:00:34]

Done.

+                         true);
   }
   completion_callback.Run(result);
 }
 
 void SimpleEntryImpl::SetSynchronousData() {
   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.
 
   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