Race Create vs SendRequest on cache misses.

net/http/http_cache.cc

Index: net/http/http_cache.cc
diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index d7461193762e31baf6c9c7608f1cc7ce345b9235..798756a7aa5e7e07520a37428162a5e455000fd4 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -30,6 +30,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/upload_data_stream.h"
 #include "net/disk_cache/disk_cache.h"
+#include "net/disk_cache/pending_create_entry.h"
 #include "net/http/http_cache_transaction.h"
 #include "net/http/http_network_layer.h"
 #include "net/http/http_network_session.h"
@@ -102,7 +103,6 @@ struct HttpCache::PendingOp {
 enum WorkItemOperation {
   WI_CREATE_BACKEND,
   WI_OPEN_ENTRY,
-  WI_CREATE_ENTRY,
   WI_DOOM_ENTRY
 };
 
@@ -712,27 +712,10 @@ int HttpCache::OpenEntry(const std::string& key, ActiveEntry** entry,
 int HttpCache::CreateEntry(const std::string& key, ActiveEntry** entry,
                            Transaction* trans) {
   DCHECK(!FindActiveEntry(key));
-
-  WorkItem* item = new WorkItem(WI_CREATE_ENTRY, trans, entry);
-  PendingOp* pending_op = GetPendingOp(key);
-  if (pending_op->writer) {
-    pending_op->pending_queue.push_back(item);
-    return ERR_IO_PENDING;
-  }
-
-  DCHECK(pending_op->pending_queue.empty());
-
-  pending_op->writer = item;
-  pending_op->callback = base::Bind(&HttpCache::OnPendingOpComplete,
-                                    AsWeakPtr(), pending_op);
-
-  int rv = disk_cache_->CreateEntry(key, &(pending_op->disk_entry),
-                                    pending_op->callback);
-  if (rv != ERR_IO_PENDING) {
-    item->ClearTransaction();
-    pending_op->callback.Run(rv);
-  }
-
+  disk_cache::Entry* new_entry;
+  int rv = disk_cache::PendingCreateEntry(disk_cache_.get(), key, &new_entry);

[rvargas (doing something else), 2013/01/23 22:04:09]

why is this not calling CreateEntry?. I mean, if you want to change the
interface again so that Create is synchronous then that's fine, and we can end
up with two methods if we don't want to cleanup all callers at the same time,
but it should be part of "the" interface. (Anyway, it is easier to change the
implementation to be sync, without changing the API).

In other words, no change should be needed in this file, at least for now.

[gavinp, 2013/01/24 07:38:10]

Good point. And if we want to do A/B tests (and we will), it's best to not
change here, or at least make the change minimal.

Hrm, I put all the code in pending_create_entry.cc/.h because it didn't really
need to take a look at any internals of backend_impl, and because I wanted to
have the code in a separate file since it was possible to do so relatively
easily.

I realise we haven't come to agreement about what operations the pending create
object should support (certainly some writes makes sense...), but other than
that, do you have any concerns about the architecture of
pending_create_entry.cc?

+  if (rv == net::OK)
+    *entry = ActivateEntry(new_entry);
   return rv;
 }
 
@@ -1000,8 +983,6 @@ void HttpCache::OnIOComplete(int result, PendingOp* pending_op) {
       entry = ActivateEntry(pending_op->disk_entry);
     } else {
       // The writer transaction is gone.
-      if (op == WI_CREATE_ENTRY)
-        pending_op->disk_entry->Doom();
       pending_op->disk_entry->Close();
       pending_op->disk_entry = NULL;
       fail_requests = true;
@@ -1043,28 +1024,7 @@ void HttpCache::OnIOComplete(int result, PendingOp* pending_op) {
       continue;
     }
 
-    if (item->operation() == WI_CREATE_ENTRY) {
-      if (result == OK) {
-        // A second Create request, but the first request succeeded.
-        item->NotifyTransaction(ERR_CACHE_CREATE_FAILURE, NULL);
-      } else {
-        if (op != WI_CREATE_ENTRY) {
-          // Failed Open followed by a Create.
-          item->NotifyTransaction(ERR_CACHE_RACE, NULL);
-          fail_requests = true;
-        } else {
-          item->NotifyTransaction(result, entry);
-        }
-      }
-    } else {
-      if (op == WI_CREATE_ENTRY && result != OK) {
-        // Failed Create followed by an Open.
-        item->NotifyTransaction(ERR_CACHE_RACE, NULL);
-        fail_requests = true;
-      } else {
-        item->NotifyTransaction(result, entry);
-      }
-    }
+    item->NotifyTransaction(result, entry);
   }
 }