Support overlapping operations on the SimpleEntryImpl.

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 70 matching lines @@
     index->Remove(key);
   WorkerPool::PostTask(FROM_HERE,
                        base::Bind(&SimpleSynchronousEntry::DoomEntry, path, key,
                                   MessageLoopProxy::current(), callback),
                        true);
   return net::ERR_IO_PENDING;
 }
 
 void SimpleEntryImpl::Doom() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(!operation_running_);
 #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
   // SimpleSynchronousEntry, and operations can even happen on them. The files
   // will be removed from the filesystem when they are closed.
   DoomEntry(index_, path_, key_, CompletionCallback());
 #else
   NOTIMPLEMENTED();
 #endif
 }
 
 void SimpleEntryImpl::Close() {
   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 (operation_running_) {

[gavinp, 2013/04/17 11:04:36]

I think if you rebase on top of https://codereview.chromium.org/13880016/ , then
this won't be necessary at all; Close() will just be safe to run.

Do you think it's a good idea to rebase this on top of that CL, and continue?

[felipeg, 2013/04/17 11:51:22]

If I am going to land before you, I think I should keep it this way and you
merge and fix.

Even with your CL, I still think it makes sense to run the Close operation in
the end of the queue.
Although I know your CL solves that problem and allows Close to run at any time,
but for the sake of consistency with the idea of a queue of operations, we
should run close as the last operation in the queue.

Also
I think your CL will not be able to CHECK(HasOneRef()) anymore, since we can
have multiple Closures enqueued to run later, holding a pointer to |this|.

[gavinp, 2013/04/17 13:13:45]

Agreed. And whoever wins the race dodges the merge. Fair point.
With my CL, it runs last always, as it's launched from the destructor! So, I
agree.
Yeah. Those checks sure didn't live long.

+    // 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;
   }
+  DCHECK(operations_.size() == 0);
+  DCHECK(!operation_running_);
+
+  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;
 }
 
 std::string SimpleEntryImpl::GetKey() const {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   return key_;
 }
 
@@ skipping 11 matching lines @@
   DCHECK(io_thread_checker_.CalledOnValidThread());
   return data_size_[index];
 }
 
 int SimpleEntryImpl::ReadData(int index,
                               int offset,
                               net::IOBuffer* buf,
                               int buf_len,
                               const CompletionCallback& callback) {
   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);
-  }
-  synchronous_entry_in_use_by_worker_ = true;
+  if (index < 0 || index >= kSimpleEntryFileCount || buf_len < 0)
+    return net::ERR_INVALID_ARGUMENT;
+  if (offset >= data_size_[index] || offset < 0 || !buf_len)
+    return 0;
+  // TODO(felipeg): Optimization: Add support for truly parallel read
+  // operations.
+  operations_.push(
+      base::Bind(&SimpleEntryImpl::ReadDataInternal,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 index,
+                 offset,
+                 make_scoped_refptr(buf),
+                 buf_len,
+                 callback));
+  RunNextOperationIfNeeded();
+  return net::ERR_IO_PENDING;
+}
+
+void SimpleEntryImpl::ReadDataInternal(int index,

[gavinp, 2013/04/17 11:04:36]

Method ordering: this should go to the end of the file.

[felipeg, 2013/04/17 11:51:22]

Done.

+                                       int offset,
+                                       scoped_refptr<net::IOBuffer> buf,
+                                       int buf_len,
+                                       const CompletionCallback& callback) {
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(!operation_running_);
+  operation_running_ = true;
   if (index_)
     index_->UseIfExists(key_);
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
                  index_, 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),
+                                  index, offset, 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) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
-  if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED();
-    CHECK(false);
-  }
-  synchronous_entry_in_use_by_worker_ = true;
+  if (index < 0 || index >= kSimpleEntryFileCount || offset < 0 || buf_len < 0)
+    return net::ERR_INVALID_ARGUMENT;
+
+  operations_.push(
+      base::Bind(&SimpleEntryImpl::WriteDataInternal,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 index,
+                 offset,
+                 make_scoped_refptr(buf),
+                 buf_len,
+                 callback,
+                 truncate));
+  RunNextOperationIfNeeded();
+
+  // TODO(felipeg): Optimization: Add support for optimistic writes, quickly
+  // returning net::OK here.
+  return net::ERR_IO_PENDING;
+}
+
+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(!operation_running_);
+  operation_running_ = true;
   if (index_)
     index_->UseIfExists(key_);
+
+  last_used_ = base::Time::Now();
+  last_modified_ = base::Time::Now();
+  data_size_[index] = buf_len;
+
   SynchronousOperationCallback sync_operation_callback =
       base::Bind(&SimpleEntryImpl::EntryOperationComplete,
                  index_, 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),
+                                  index, offset, buf,
                                   buf_len, sync_operation_callback, truncate),
                        true);
-  return net::ERR_IO_PENDING;
 }
 
 int SimpleEntryImpl::ReadSparseData(int64 offset,
                                     net::IOBuffer* buf,
                                     int buf_len,
                                     const CompletionCallback& callback) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // TODO(gavinp): Determine if the simple backend should support sparse data.
   NOTIMPLEMENTED();
   return net::ERR_FAILED;
@@ skipping 38 matching lines @@
   return net::ERR_FAILED;
 }
 
 SimpleEntryImpl::SimpleEntryImpl(
     SimpleSynchronousEntry* synchronous_entry,
     WeakPtr<SimpleIndex> index)
     : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
       path_(synchronous_entry->path()),
       key_(synchronous_entry->key()),
       synchronous_entry_(synchronous_entry),
-      synchronous_entry_in_use_by_worker_(false),
+      operation_running_(false),
       index_(index) {
   DCHECK(synchronous_entry);
   SetSynchronousData();
 }
 
 SimpleEntryImpl::~SimpleEntryImpl() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
 }
 
 // static
@@ skipping 25 matching lines @@
     int result) {
   DCHECK(sync_entry);
   if (index) {
     if (result >= 0)
       index->UpdateEntrySize(sync_entry->key(), sync_entry->GetFileSize());
     else
       index->Remove(sync_entry->key());
   }
 
   if (entry) {
-    DCHECK(entry->synchronous_entry_in_use_by_worker_);
-    entry->synchronous_entry_in_use_by_worker_ = false;
+    DCHECK(entry->operation_running_);
+    entry->operation_running_ = false;
     entry->SetSynchronousData();
+    entry->RunNextOperationIfNeeded();
   } 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)),
                          true);
   }
   completion_callback.Run(result);
 }
 
+bool SimpleEntryImpl::RunNextOperationIfNeeded() {
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  if (operations_.size() <= 0 || operation_running_)
+    return false;
+  base::Closure operation = operations_.front();
+  operations_.pop();
+  operation.Run();
+  return true;
+}
+
 void SimpleEntryImpl::SetSynchronousData() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
-  DCHECK(!synchronous_entry_in_use_by_worker_);
+  DCHECK(!operation_running_);
   // 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