Race Create vs SendRequest on cache misses.

net/http/http_cache.cc

 // Copyright (c) 2012 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/http/http_cache.h"
 
 #include <algorithm>
 
 #include "base/compiler_specific.h"
 
@@ skipping 12 matching lines @@
 #include "base/pickle.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #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"
 #include "net/http/http_request_info.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_response_info.h"
 #include "net/http/http_util.h"
 
 namespace net {
 
@@ skipping 52 matching lines @@
   CompletionCallback callback;  // BackendCallback.
   WorkItemList pending_queue;
 };
 
 //-----------------------------------------------------------------------------
 
 // The type of operation represented by a work item.
 enum WorkItemOperation {
   WI_CREATE_BACKEND,
   WI_OPEN_ENTRY,
-  WI_CREATE_ENTRY,
   WI_DOOM_ENTRY
 };
 
 // A work item encapsulates a single request to the backend with all the
 // information needed to complete that request.
 class HttpCache::WorkItem {
  public:
   WorkItem(WorkItemOperation operation, Transaction* trans, ActiveEntry** entry)
       : operation_(operation),
         trans_(trans),
@@ skipping 589 matching lines @@
     item->ClearTransaction();
     pending_op->callback.Run(rv);
   }
 
   return rv;
 }
 
 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;
 }
 
 void HttpCache::DestroyEntry(ActiveEntry* entry) {
   if (entry->doomed) {
     FinalizeDoomedEntry(entry);
   } else {
     DeactivateEntry(entry);
   }
 }
@@ skipping 247 matching lines @@
   std::string key;
   if (result == OK) {
     if (op == WI_DOOM_ENTRY) {
       // Anything after a Doom has to be restarted.
       fail_requests = true;
     } else if (item->IsValid()) {
       key = pending_op->disk_entry->GetKey();
       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;
     }
   }
 
   // We are about to notify a bunch of transactions, and they may decide to
   // re-issue a request (or send a different one). If we don't delete
   // pending_op, the new request will be appended to the end of the list, and
   // we'll see it again from this point before it has a chance to complete (and
@@ skipping 21 matching lines @@
       entry = FindActiveEntry(key);
       if (!entry)
         fail_requests = true;
     }
 
     if (fail_requests) {
       item->NotifyTransaction(ERR_CACHE_RACE, NULL);
       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);
   }
 }
 
 // static
 void HttpCache::OnPendingOpComplete(const base::WeakPtr<HttpCache>& cache,
                                     PendingOp* pending_op,
                                     int rv) {
   if (cache.get()) {
     cache->OnIOComplete(rv, pending_op);
   } else {
@@ skipping 38 matching lines @@
     building_backend_ = false;
     DeletePendingOp(pending_op);
   }
 
   // The cache may be gone when we return from the callback.
   if (!item->DoCallback(result, backend))
     item->NotifyTransaction(result, NULL);
 }
 
 }  // namespace net