Support overlapping operations on the SimpleEntryImpl.

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 1917a88715091ac1a37be9dae3b8fd104b6f4edf..f9fea2a9662f08554e46c636507907c4060400f4 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -87,7 +87,28 @@ int SimpleEntryImpl::DoomEntry(WeakPtr<SimpleIndex> index,
 }
 
 void SimpleEntryImpl::Doom() {

[gavinp, 2013/04/17 08:12:35]

I think doom is one of the only operations that already can be run completely
concurrently with anything else (including other Dooms).

[felipeg, 2013/04/17 10:06:43]

Done.

+  LOG(INFO) << " DOOM " << key_;
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  if (synchronous_entry_in_use_by_worker_) {
+    // Postpone doom operation.
+    operations_.push(
+        base::Bind(&SimpleEntryImpl::DoomInternal,
+                   weak_ptr_factory_.GetWeakPtr()));
+    return;
+  }
+  DCHECK(operations_.size() == 0);
+  DCHECK(!synchronous_entry_in_use_by_worker_);
+
+  DoomInternal();
+  // TODO(felipeg): Maybe we should have a flag is_doomed_ and do some DCHECKS
+  // around the code, to see if we dont call read in a doomed entry.

[gavinp, 2013/04/17 08:12:35]

I believe it's correct and valid to Read/Write in a doomed entry.

[felipeg, 2013/04/17 10:06:43]

Done.

+  RunNextOperationIfNeeded();
+}
+
+void SimpleEntryImpl::DoomInternal() {
+  LOG(INFO) << " DOOM " << key_;
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(!synchronous_entry_in_use_by_worker_);
 #if defined(OS_POSIX)
   // This call to static SimpleEntryImpl::DoomEntry() will just erase the
   // underlying files. On POSIX, this is fine; the files are still open on the
@@ -100,13 +121,26 @@ void SimpleEntryImpl::Doom() {
 }
 
 void SimpleEntryImpl::Close() {
+  LOG(INFO) << " CLOSE " << key_;
   DCHECK(io_thread_checker_.CalledOnValidThread());
-  if (!synchronous_entry_in_use_by_worker_) {
-    WorkerPool::PostTask(FROM_HERE,
-                         base::Bind(&SimpleSynchronousEntry::Close,
-                                    base::Unretained(synchronous_entry_)),
-                         true);
+  if (synchronous_entry_in_use_by_worker_) {
+    LOG(INFO) << " CLOSE " << key_;
+    // Postpone close operation.
+    // Push the close operation to the end of the line. This way we run all
+    // operations before we are able close.
+    operations_.push(
+        base::Bind(&SimpleEntryImpl::Close,
+                   weak_ptr_factory_.GetWeakPtr()));
+    return;
   }
+  LOG(INFO) << " CLOSE " << key_;
+  DCHECK(operations_.size() == 0);
+  DCHECK(!synchronous_entry_in_use_by_worker_);
+
+  WorkerPool::PostTask(FROM_HERE,
+                       base::Bind(&SimpleSynchronousEntry::Close,
+                                  base::Unretained(synchronous_entry_)),
+                       true);
   // Entry::Close() is expected to release this entry. See disk_cache.h for
   // details.
   delete this;
@@ -132,20 +166,49 @@ 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) {
+  LOG(INFO) << " READ " << key_;
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // 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. This might make calling SimpleSynchronousEntry::Close()
   // correctly more tricky (see SimpleEntryImpl::EntryOperationComplete).
-  if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED();
-    CHECK(false);
+  if (synchronous_entry_in_use_by_worker_ && !read_only_operation_) {
+    // Postpone read operation.
+    operations_.push(
+        base::Bind(&SimpleEntryImpl::ReadDataInternal,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   index,
+                   offset,
+                   make_scoped_refptr(buf),
+                   buf_len,
+                   callback));
+    return net::ERR_IO_PENDING;
   }
+  DCHECK(operations_.size() == 0);
+  ReadDataInternal(index,
+                   offset,
+                   make_scoped_refptr(buf),
+                   buf_len,
+                   callback);
+  return net::ERR_IO_PENDING;
+}
+
+void SimpleEntryImpl::ReadDataInternal(int index,
+                                       int offset,
+                                       scoped_refptr<net::IOBuffer> buf,
+                                       int buf_len,
+                                       const CompletionCallback& callback) {
+  LOG(INFO) << " READ " << key_;
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(!synchronous_entry_in_use_by_worker_);
+  read_only_operation_ = true;
   synchronous_entry_in_use_by_worker_ = true;
   if (index_)
     index_->UseIfExists(key_);
@@ -156,10 +219,14 @@ int SimpleEntryImpl::ReadData(int index,
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::ReadData,
                                   base::Unretained(synchronous_entry_),
-                                  index, offset, make_scoped_refptr(buf),
+                                  index, offset, buf,

[gavinp, 2013/04/17 08:17:14]

This is really dangerous, calling with the naked buf. The problem is that the
caller can drop their ref to buf, and then buf is deleted under us. The existing
calls used make_scoped_refptr(), which gets copied into the Closure() and again
into the thread when it runs the Closure.

[gavinp, 2013/04/17 09:40:16]

Boy this stuff is subtle.

I was wrong: I failed to notice that ReadDataInternal's argument is a
scoped_refptr<>, so the Closure will copy _THAT_, so it's all good.

                                   buf_len, sync_operation_callback),
                        true);
-  return net::ERR_IO_PENDING;
+}
+
+// TODO(felipeg): Maybe in case of failure from a write operation, we could try
+// to invalidate all operations in the queue of the current entry.
+static void WriteDataNoOp(int noop) {
 }
 
 int SimpleEntryImpl::WriteData(int index,

[gavinp, 2013/04/17 09:23:56]

I think ReadData / WriteData can be made a lot simpler. How about this:

- Always push the Closure onto operations_, regardless of
synchronous_entry_in_use_by_worker_.
- Always call RunNextOperationIfNeeded.

(RunNextOperationIfNeeded should check synchronous_entry_in_use_by_worker_)

[felipeg, 2013/04/17 10:06:43]

Done.

@@ -168,14 +235,56 @@ int SimpleEntryImpl::WriteData(int index,
                                int buf_len,
                                const CompletionCallback& callback,
                                bool truncate) {
+  LOG(INFO) << " WRITE " << key_ << "  buf " << buf << "    buf_len " << buf_len << "    truncate " << truncate;
   DCHECK(io_thread_checker_.CalledOnValidThread());
+
   if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED();
-    CHECK(false);
+    LOG(INFO) << " WRITE " << key_;
+    // Postpone write operation.
+    operations_.push(
+        base::Bind(&SimpleEntryImpl::WriteDataInternal,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   index,
+                   offset,
+                   make_scoped_refptr(buf),
+                   buf_len,
+                   callback,
+                   truncate));
+    // When writting, if there is already a pending operation, we have to return
+    // pending.
+    return net::ERR_IO_PENDING;
   }
+  LOG(INFO) << " WRITE " << key_;
+  DCHECK(operations_.size() == 0);
+  WriteDataInternal(index,
+                    offset,
+                    make_scoped_refptr(buf),
+                    buf_len,
+                    //callback,
+                    base::Bind(&WriteDataNoOp),
+                    truncate);
+  //return net::ERR_IO_PENDING;
+  // Pretend everything goes fine, and make HTTP Cache Transaction happy.
+  return buf_len;
+}
+
+void SimpleEntryImpl::WriteDataInternal(int index,
+                                       int offset,
+                                       scoped_refptr<net::IOBuffer> buf,
+                                       int buf_len,
+                                       const CompletionCallback& callback,
+                                       bool truncate) {
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(!synchronous_entry_in_use_by_worker_);
+  read_only_operation_ = false;
   synchronous_entry_in_use_by_worker_ = true;
   if (index_)
     index_->UseIfExists(key_);
+
+  // TODO(felipeg):   last_used_  last_modified_
+
+  data_size_[index] = buf_len;
+
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
                  index_, callback, weak_ptr_factory_.GetWeakPtr(),
@@ -183,10 +292,10 @@ int SimpleEntryImpl::WriteData(int index,
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::WriteData,
                                   base::Unretained(synchronous_entry_),
-                                  index, offset, make_scoped_refptr(buf),
+                                  index, offset, buf,
                                   buf_len, sync_operation_callback, truncate),
                        true);
-  return net::ERR_IO_PENDING;
+  LOG(INFO) << " WRITE " << key_;
 }
 
 int SimpleEntryImpl::ReadSparseData(int64 offset,
@@ -246,6 +355,7 @@ SimpleEntryImpl::SimpleEntryImpl(
       key_(synchronous_entry->key()),
       synchronous_entry_(synchronous_entry),
       synchronous_entry_in_use_by_worker_(false),
+      read_only_operation_(false),
       index_(index) {
   DCHECK(synchronous_entry);
   SetSynchronousData();
@@ -291,10 +401,21 @@ void SimpleEntryImpl::EntryOperationComplete(
   }
 
   if (entry) {
+    if (entry->read_only_operation_)
+      LOG(INFO) << " READ EntryOperationComplete " << entry->key();
+    else
+      LOG(INFO) << " WRITE EntryOperationComplete " << entry->key();
     DCHECK(entry->synchronous_entry_in_use_by_worker_);
+    entry->read_only_operation_ = false;
     entry->synchronous_entry_in_use_by_worker_ = false;
     entry->SetSynchronousData();
+
+    entry->RunNextOperationIfNeeded();
+
+    completion_callback.Run(result);
   } else {
+
+    LOG(INFO) << " EntryOperationComplete " << entry->key();
     // |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|.
@@ -302,8 +423,23 @@ void SimpleEntryImpl::EntryOperationComplete(
                          base::Bind(&SimpleSynchronousEntry::Close,
                                     base::Unretained(sync_entry)),
                          true);
+
+    completion_callback.Run(result);
   }
-  completion_callback.Run(result);
+}
+
+void SimpleEntryImpl::RunNextOperationIfNeeded() {
+  LOG(INFO) << " RunNextOperationIfNeeded " << key_;
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  if (operations_.size() <= 0)
+    return;
+  base::Closure operation = operations_.front();
+  operations_.pop();
+
+  LOG(INFO) << " operation.Run " << key_;
+  // TODO(felipeg): maybe we should post task. But it could shuffle even more
+  // the order of new operations in this entry.

[gavinp, 2013/04/17 08:17:14]

The callers should be totally insensitive to the order of operations. However, I
say no post: this should be cheap anyway right?

[felipeg, 2013/04/17 10:06:43]

No post is better because when debugging our minds won't blow.

+  operation.Run();
 }
 
 void SimpleEntryImpl::SetSynchronousData() {