Server push cancellation: add a new class HttpCacheLookupManager which implements ServerPushDelegate

net/http/http_cache_lookup_manager.cc

Index: net/http/http_cache_lookup_manager.cc
diff --git a/net/http/http_cache_lookup_manager.cc b/net/http/http_cache_lookup_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3de8cade56ea51c59fa22b711ba1ab6fe12a0548
--- /dev/null
+++ b/net/http/http_cache_lookup_manager.cc
@@ -0,0 +1,69 @@
+// Copyright 2016 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_lookup_manager.h"
+
+#include "net/base/load_flags.h"
+
+namespace net {
+
+HttpCacheLookupManager::LookupTransaction::LookupTransaction(
+    std::unique_ptr<ServerPushHelper> server_push_helper)
+    : transaction(nullptr) {
+  push_helper = std::move(server_push_helper);
+  request.reset(new HttpRequestInfo());

[Ryan Hamilton, 2016/11/15 23:16:06]

nit: you can do both of these in the initializer list.

[Zhongyi Shi, 2016/11/16 21:04:42]

Done.

+  request->url = push_helper->GetURL();
+  request->method = "GET";

[Ryan Hamilton, 2016/11/15 23:16:06]

Is GET the only method which can be pushed?

[Zhongyi Shi, 2016/11/16 21:04:42]

I believe both "GET" and "HEAD" can be pushed for QUIC, and only "GET" for
SPDY(https://cs.chromium.org/chromium/src/net/spdy/spdy_http_stream.cc?sq=package:chromium&rcl=1479284732&l=75).

+  request->load_flags = LOAD_ONLY_FROM_CACHE | LOAD_SKIP_CACHE_VALIDATION;

[Ryan Hamilton, 2016/11/15 23:16:06]

I'm not super familiar with all the details of the cache. Why skip validation?

[Zhongyi Shi, 2016/11/16 21:04:42]

Cauz this is the step 1: we only want to check whether the url is cached in the
HttpCache. Do we want to validating the cache entry?
https://cs.chromium.org/chromium/src/net/http/http_cache_transaction.cc?sq=pa...

[Ryan Hamilton, 2016/11/17 22:47:24]

Ah! I wasn't actually sure what validation meant there :> I was thinking "is the
cache corrupt", which is totally wrong! How 'bout a comment?

+}
+
+HttpCacheLookupManager::LookupTransaction::~LookupTransaction() {}
+
+HttpCacheLookupManager::HttpCacheLookupManager(HttpCache* http_cache,
+                                               const NetLogWithSource& net_log)
+    : net_log_(net_log), http_cache_(http_cache), weak_factory_(this) {}
+
+HttpCacheLookupManager::~HttpCacheLookupManager() {}
+
+void HttpCacheLookupManager::OnPush(
+    std::unique_ptr<ServerPushHelper> push_helper) {
+  GURL pushed_url = push_helper->GetURL();
+
+  // There's a pending lookup transaction sent over already, cancel this one.
+  if (lookup_transactions_.find(pushed_url) != lookup_transactions_.end())
+    return;

[Ryan Hamilton, 2016/11/15 23:16:06]

Why does simply returning cancel this push? Is that 'cause of the destructor of
ServerPushHelper?

[Zhongyi Shi, 2016/11/16 21:04:42]

Sorry for the confusion, I meant to say cancel send out the lookup transaction.
We won't cancel this push but cancel the lookup for this push. 

+
+  std::unique_ptr<LookupTransaction> lookup_transaction(
+      new LookupTransaction(std::move(push_helper)));

[Ryan Hamilton, 2016/11/15 23:16:06]

nit: auto lookup = base::MakeUnique<LookupTransaction>(std::move(push_helper));

[Zhongyi Shi, 2016/11/16 21:04:42]

Done.

+  int rv = http_cache_->CreateTransaction(DEFAULT_PRIORITY,
+                                          &lookup_transaction->transaction);
+  if (rv != OK)
+    return;
+
+  HttpTransaction* trans_ptr = lookup_transaction->transaction.get();
+  HttpRequestInfo* request_ptr = lookup_transaction->request.get();
+
+  lookup_transactions_[pushed_url] = std::move(lookup_transaction);

[Ryan Hamilton, 2016/11/15 23:16:06]

Perhaps this should only be done if Start returns ERR_IO_PENDING?

[Zhongyi Shi, 2016/11/16 21:04:42]

Ah, good suggestion! I then could skip storing in the ERR_CACHE_MISS case.

+
+  rv = trans_ptr->Start(
+      request_ptr, base::Bind(&HttpCacheLookupManager::OnPushFilteringComplete,
+                              weak_factory_.GetWeakPtr(), pushed_url),
+      net_log_);

[Ryan Hamilton, 2016/11/15 23:16:05]

I think it might be simpler to move all of this logic into LookupTransaction and
have it expose something like

int StartLookup(HttpCache* cache, CompletionCallback callback);

This would both create the transaction and start it. (You'd also need to make
LookupTransaction a class, but I think that would be a good idea anyway.

[Zhongyi Shi, 2016/11/16 21:04:42]

It's hard because in that case we will have the manager having a map storing the
lookup_transactions(which is already what we are doing now). And the lookup
transaction will then need a extra pointer to manager to notify the manager when
the callback has been invoked and the lookup transaction can be deleted from the
map. If that doesn't sound problematic, I am fine to move in that direction.

[Ryan Hamilton, 2016/11/17 22:47:24]

I may totally be missing the problem you're thinking about. I was imagining:

auto lookup = base::MakeUnique<LookupTransaction>(std::move(push_helper));
int rv  = lookup->StartLookup(http_cache_, base::Bind(...));

if (rv == ERR_IO_PENDING) {
 lookup_transactions_[pushed_url] = std::move(lookup);
}

Would that work?

That being said, the code here is much simpler now so keeping it the way you
have it now is also fine.

[Zhongyi Shi, 2016/11/18 20:34:19]

Acknowledged.

+
+  if (rv == ERR_CACHE_MISS) {

[Ryan Hamilton, 2016/11/15 23:16:06]

What if start returns some other error?

[Zhongyi Shi, 2016/11/16 21:04:42]

I think wither ERR_IO_PENDING or ERR_CACHE_MISS. ERR_IO_PENDING eventually will
call OnPushFilteringComplete which also removes the lookup from the map.
However, I now just changed the code to only store the lookup if ERR_IO_PENDING.

[Ryan Hamilton, 2016/11/17 22:47:24]

I think that makes sense. The callback will only be invoked after this method
returns ERR_IO_PENDING. So if it returns anything else, the callback will not be
invoked and we need to cleanup now.

[Zhongyi Shi, 2016/11/18 20:34:19]

Acknowledged.

+    lookup_transactions_.erase(pushed_url);
+  }
+}
+
+void HttpCacheLookupManager::OnPushFilteringComplete(const GURL& url, int rv) {
+  auto it = lookup_transactions_.find(url);
+  DCHECK(it != lookup_transactions_.end());
+
+  if (rv == OK)
+    it->second->push_helper->Cancel();
+
+  lookup_transactions_.erase(it);
+}
+
+}  // namespace net