Add new simple disk cache backend.

net/disk_cache/very_simple/very_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/very_simple/very_simple_entry_impl.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/message_loop_proxy.h"
+#include "base/threading/worker_pool.h"
+#include "net/base/net_errors.h"
+#include "net/disk_cache/very_simple/very_simple_synchronous_entry.h"
+
+namespace disk_cache {
+
+using base::Bind;
+using base::Callback;
+using base::FilePath;
+using base::MessageLoopProxy;
+using base::Owned;
+using base::Time;
+using base::Unretained;
+using base::WorkerPool;
+
+namespace {

[rvargas (doing something else), 2013/02/06 03:28:40]

nit: can this be out of disk_cache?

[gavinp, 2013/02/08 23:17:51]

Done.

+
+typedef Entry::CompletionCallback CompletionCallback;
+typedef VerySimpleSynchronousEntry::SynchronousEntryCallback
+    SynchronousEntryCallback;
+typedef Callback<VerySimpleEntryImpl*(VerySimpleSynchronousEntry*)>
+    EntryFactoryCallback;
+typedef Callback<void(VerySimpleSynchronousEntry*)> SetSyncCallback;
+
+// PendingCreationOperation represents a pending operation that is creating a
+// new VerySimpleSynchronousEntry, such as OpenEntry() or CreateEntry(). After
+// the new synchronous entry has been created, it uses its factory to create
+// a VerySimpleEntryImpl and pass it back to the original caller.
+class PendingCreationOperation {
+ public:
+  static SynchronousEntryCallback Create(
+      const EntryFactoryCallback& entry_factory_callback,
+      const CompletionCallback& completion_callback,
+      Entry** out_entry) {
+    PendingCreationOperation* operation =

[rvargas (doing something else), 2013/02/06 03:28:40]

Could you separate the implementation from the declaration of this class?
(declaration on this file is totally fine)

[gavinp, 2013/02/08 23:17:51]

Done.

+        new PendingCreationOperation(entry_factory_callback,
+                                     completion_callback,
+                                     out_entry);
+    DCHECK(operation);
+    return Bind(&PendingCreationOperation::OnIOComplete,
+                Owned(operation));
+  }
+
+ private:
+  PendingCreationOperation(const EntryFactoryCallback& entry_factory_callback,
+                           const CompletionCallback& completion_callback,
+                           Entry** out_entry)
+      : entry_factory_callback_(entry_factory_callback),
+        completion_callback_(completion_callback),
+        out_entry_(out_entry) {
+  }
+
+  void OnIOComplete(VerySimpleSynchronousEntry* sync_entry, int result) {
+    DCHECK_NE(net::ERR_IO_PENDING, result);
+
+    if (result != net::OK) {
+      DCHECK_EQ(static_cast<VerySimpleSynchronousEntry*>(NULL), sync_entry);

[rvargas (doing something else), 2013/02/06 03:28:40]

nit: DCHECK(!sync_entry)

[gavinp, 2013/02/08 23:17:51]

Done. I had to add some printing cruft, since otherwise debugging output would
suffer. What a shame that DCHECK_EQ(NULL, foo) doesn't work.

+      completion_callback_.Run(result);
+      return;
+    }
+    DCHECK(sync_entry);
+    *out_entry_ = entry_factory_callback_.Run(sync_entry);

[rvargas (doing something else), 2013/02/06 03:28:40]

I'm not so happy with this pattern of run a task and pass another task that
should be executed on the callback, before running the user callback :(

Why not a simple PostTaskAndReply, where the control flow is straightforward (at
the cost of an extra method for the continuation... that in this case would be
basically this method)?

[gavinp, 2013/02/08 23:17:51]

I'm sorry to hear you're unhappy with this pattern. Why are you unhappy with
this pattern? Can you explain what is wrong with it? It can be very difficult to
respond to a short comment that says something is bad or wrong; especially when
I don't understand why. I will try and elaborate on my thinking below. I hope it
helps.

I think it would be subtle to get thread safety right if we had the
SynchronousEntryImpl directly called a task provided to the backend. I also
don't understand why that is virtuous.

In the case of creating an entry, I used the PendingCreationOperation for two
goals. I wanted to delay creating the SimpleEntryImpl object until as late as
possible, and provide it to the caller's Entry** as late as possible, to avoid
there being a concept of an uninitialized SimpleEntryImpl and its required
checks. By creating it in the callback, the SimpleEntryImpl is constructed with
a valid open SynchronousEntry or it is not constructed. Since PostTaskAndReply
requires that the reply be a Closure, this pattern would not work here: how
would the SynchronousEntryImpl provide its pointer to the reply?

I guess we could create the SynchronousEntryImpl on the IO thread, and construct
our SimpleEntryImpl early, and now we have this uninitialized state I was trying
to avoid. I think that is uglier than creating a limited scope callback that
follows up on completed operations.

In the case of ReadData() and WriteData(), we could use PostTaskAndReply if we
kept our SimpleSynchronousEntry pointer, but then we'd be introducing an
interesting class of data races. The reply necessarily occurs after the worker
thread task is complete, so the callback has already run on the worker thread.

To make this code less LISP like, and more C++ like, I've moved the
PendingOperation and PendingCreationOperation to inner classes, and made them
friends.

+    DCHECK(*out_entry_);
+    completion_callback_.Run(net::OK);
+  }
+
+  EntryFactoryCallback entry_factory_callback_;
+  CompletionCallback completion_callback_;
+  Entry** out_entry_;
+};
+
+class PendingOperation {

[rvargas (doing something else), 2013/02/06 03:28:40]

nit: class description

[gavinp, 2013/02/08 23:17:51]

Done.

+ public:
+  static SynchronousEntryCallback Create(
+      const SetSyncCallback& set_sync_callback,
+      const CompletionCallback& completion_callback) {
+    PendingOperation* operation = new PendingOperation(set_sync_callback,

[rvargas (doing something else), 2013/02/06 03:28:40]

ditto (implementation vs declaration)

[gavinp, 2013/02/08 23:17:51]

Done.

+                                                       completion_callback);
+    DCHECK(operation);
+    return Bind(&PendingOperation::OnIOComplete, Owned(operation));
+  }
+
+ private:
+  PendingOperation(const SetSyncCallback& set_sync_callback,
+                   const CompletionCallback& completion_callback)
+      : set_sync_callback_(set_sync_callback),
+        completion_callback_(completion_callback) {
+  }
+
+  void OnIOComplete(VerySimpleSynchronousEntry* sync_entry, int result) {
+    set_sync_callback_.Run(sync_entry);

[rvargas (doing something else), 2013/02/06 03:28:40]

ditto

+    completion_callback_.Run(result);
+  }
+
+  SetSyncCallback set_sync_callback_;
+  CompletionCallback completion_callback_;
+};
+
+}  // namespace
+
+// static
+int VerySimpleEntryImpl::OpenEntry(const FilePath& path,
+                                   const std::string& key,
+                                   Entry** entry,
+                                   const CompletionCallback& callback) {
+  SynchronousEntryCallback sync_entry_callback =
+      PendingCreationOperation::Create(
+          Bind(&VerySimpleEntryImpl::Create), callback, entry);
+  WorkerPool::PostTask(FROM_HERE,

[rvargas (doing something else), 2013/02/06 03:28:40]

You know that in general we don't like the worker pool for anything other than
very basic, pure OS related stuff, right?

[gavinp, 2013/02/08 23:17:51]

I don't know this. Can you expand on this?

[rvargas (doing something else), 2013/02/13 01:48:46]

That is documented on the header of worker pool.

The basic reasoning is that the native worker pool has automatic thread
management (which in general is a good thing), but it prevents us from
controlling what happens with elements on the queue (and currently running) at
shutdown. At such, we run into issues with anything else than very simple tasks.

That's the reason we developed our own thread pool infrastructure.

+                       Bind(&VerySimpleSynchronousEntry::OpenEntry, path, key,
+                            MessageLoopProxy::current(),
+                            sync_entry_callback),
+                       true);
+  return net::ERR_IO_PENDING;
+}
+
+// static
+int VerySimpleEntryImpl::CreateEntry(const FilePath& path,
+                                      const std::string& key,
+                                      Entry** entry,
+                                      const CompletionCallback& callback) {
+  SynchronousEntryCallback sync_entry_callback =
+      PendingCreationOperation::Create(
+          Bind(&VerySimpleEntryImpl::Create), callback, entry);
+  WorkerPool::PostTask(FROM_HERE,
+                       Bind(&VerySimpleSynchronousEntry::CreateEntry, path, key,
+                            MessageLoopProxy::current(),
+                            sync_entry_callback),
+                       true);
+  return net::ERR_IO_PENDING;
+}
+
+// static
+int VerySimpleEntryImpl::DoomEntry(const FilePath& path,
+                                   const std::string& key,
+                                   const CompletionCallback& callback) {
+  WorkerPool::PostTask(FROM_HERE,
+                       Bind(&VerySimpleSynchronousEntry::DoomEntry, path, key,
+                            MessageLoopProxy::current(), callback),
+                       true);
+  return net::ERR_IO_PENDING;
+}
+
+void VerySimpleEntryImpl::Doom() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  WorkerPool::PostTask(FROM_HERE,
+                       Bind(&VerySimpleSynchronousEntry::DoomAndClose,

[rvargas (doing something else), 2013/02/06 03:28:40]

DoomAndClose?

[gavinp, 2013/02/08 23:17:51]

DoomAndClose!

[rvargas (doing something else), 2013/02/13 01:48:46]

I meant it was surprising behavior. As far as I can see it is also incorrect in
that an entry should remain 100% functional after doom.

+                            Unretained(ReleaseSynchronousEntry())),
+                       true);
+  has_been_doomed_ = true;
+}
+
+void VerySimpleEntryImpl::Close() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!has_been_doomed_) {
+    WorkerPool::PostTask(FROM_HERE,
+                         Bind(&VerySimpleSynchronousEntry::Close,
+                              Unretained(ReleaseSynchronousEntry())),
+                         true);
+  }
+  // Entry::Close() is expected to release this entry. See disk_cache.h for
+  // details.
+  delete this;
+}
+
+std::string VerySimpleEntryImpl::GetKey() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return key_;
+}
+
+Time VerySimpleEntryImpl::GetLastUsed() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
+  return synchronous_entry_->last_used();
+}
+
+Time VerySimpleEntryImpl::GetLastModified() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
+  return synchronous_entry_->last_modified();
+}
+
+int32 VerySimpleEntryImpl::GetDataSize(int index) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
+  return synchronous_entry_->data_size(index);
+}
+
+int VerySimpleEntryImpl::ReadData(int index,
+                                  int offset,
+                                  net::IOBuffer* buf,
+                                  int buf_len,
+                                  const CompletionCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
+  SynchronousEntryCallback sync_entry_callback =
+      PendingOperation::Create(Bind(&VerySimpleEntryImpl::SetSynchronousEntry,
+                                    weak_ptr_factory_.GetWeakPtr()),
+                               callback);
+  WorkerPool::PostTask(FROM_HERE,
+                       Bind(&VerySimpleSynchronousEntry::ReadData,
+                            Unretained(ReleaseSynchronousEntry()), index,

[rvargas (doing something else), 2013/02/06 03:28:40]

is this intended to enforce exclusive access to simple members? The interface
doesn't prevent issuing multiple simultaneous reads.

[gavinp, 2013/02/08 23:17:51]

This is intended to enforce thread safety, and the easiest way to do that was to
enforce exclusive access using the memory barrier in PostTask for safety. The
use of the WorkerPool in particular makes this work well.

Yes, the interface does not prevent issuing multiple simultaneous reads. In your
comment, you do not elaborate about where multiple simultaneous reads occurs
when in an HttpCache::Transaction, but that can and does occur when an existing
transaction has an HttpCache::ActiveEntry open only for read; a second reader is
allowed in.

This DCHECK will crash when that happens. I have added a TODO.

+                            offset, Unretained(buf), buf_len,

[rvargas (doing something else), 2013/02/06 03:28:40]

cannot be unretained

[gavinp, 2013/02/08 23:17:51]

Tell me more?

[rvargas (doing something else), 2013/02/13 01:48:46]

that would mean keeping the buffer without grabbing a reference to it.

+                            sync_entry_callback),
+                       true);
+  return net::ERR_IO_PENDING;
+}
+
+int VerySimpleEntryImpl::WriteData(int index,
+                                   int offset,
+                                   net::IOBuffer* buf,
+                                   int buf_len,
+                                   const CompletionCallback& callback,
+                                   bool truncate) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
+  SynchronousEntryCallback sync_entry_callback =
+      PendingOperation::Create(Bind(&VerySimpleEntryImpl::SetSynchronousEntry,
+                                    weak_ptr_factory_.GetWeakPtr()),
+                               callback);
+  WorkerPool::PostTask(FROM_HERE,
+                       Bind(&VerySimpleSynchronousEntry::WriteData,
+                            Unretained(ReleaseSynchronousEntry()), index,
+                            offset, Unretained(buf), buf_len,
+                            sync_entry_callback, truncate),
+                       true);
+  return net::ERR_IO_PENDING;
+}
+
+int VerySimpleEntryImpl::ReadSparseData(int64 offset,
+                                        net::IOBuffer* buf,
+                                        int buf_len,
+                                        const CompletionCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return net::ERR_FAILED;
+}
+
+int VerySimpleEntryImpl::WriteSparseData(int64 offset,
+                                         net::IOBuffer* buf,
+                                         int buf_len,
+                                         const CompletionCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return net::ERR_FAILED;
+}
+
+int VerySimpleEntryImpl::GetAvailableRange(int64 offset,
+                                           int len,
+                                           int64* start,
+                                           const CompletionCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return net::ERR_FAILED;
+}
+
+bool VerySimpleEntryImpl::CouldBeSparse() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return false;
+}
+
+void VerySimpleEntryImpl::CancelSparseIO() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+int VerySimpleEntryImpl::ReadyForSparseIO(const CompletionCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return net::ERR_FAILED;
+}
+
+VerySimpleEntryImpl::VerySimpleEntryImpl(
+    VerySimpleSynchronousEntry* synchronous_entry)
+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
+      key_(synchronous_entry->key()),

[rvargas (doing something else), 2013/02/06 03:28:40]

why duplicate the key?

[gavinp, 2013/02/08 23:17:51]

Thread safe access. In practice, if SimpleSynchronousEntry::key_ was const, we
could retain a pointer to it and use that across threads safely. However, the
copy was conceptually simpler for me to deal with and didn't require that
argument.

Do you have another suggestion?

[rvargas (doing something else), 2013/02/13 01:48:46]

The code is happy to reach into access times and sizes(from the synchronous
entry) and the key is not much different.

+      synchronous_entry_(synchronous_entry),
+      has_been_doomed_(false) {
+  DCHECK(synchronous_entry);
+}
+
+VerySimpleEntryImpl::~VerySimpleEntryImpl() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(static_cast<VerySimpleSynchronousEntry*>(NULL), synchronous_entry_);

[rvargas (doing something else), 2013/02/06 03:28:40]

!sync_entry_

[gavinp, 2013/02/08 23:17:51]

Done.

+}
+
+// static
+VerySimpleEntryImpl* VerySimpleEntryImpl::Create(
+    VerySimpleSynchronousEntry* synchronous_entry) {
+  return new VerySimpleEntryImpl(synchronous_entry);
+}
+
+VerySimpleSynchronousEntry* VerySimpleEntryImpl::ReleaseSynchronousEntry() {
+  DCHECK(synchronous_entry_);
+  VerySimpleSynchronousEntry* retval = synchronous_entry_;
+  synchronous_entry_ = NULL;
+  return retval;
+}
+
+void VerySimpleEntryImpl::SetSynchronousEntry(
+    VerySimpleSynchronousEntry* synchronous_entry) {
+  DCHECK_EQ(static_cast<VerySimpleSynchronousEntry*>(NULL), synchronous_entry_);
+  synchronous_entry_ = synchronous_entry;
+}
+
+}  // namespace disk_cache