stale-while-revalidate experimental implementation.

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"
 
 #if defined(OS_POSIX)
 #include <unistd.h>
 #endif
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial.h"
+#include "base/metrics/histogram.h"
 #include "base/pickle.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/worker_pool.h"
+#include "base/time/time.h"
 #include "net/base/cache_type.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
+#include "net/base/network_delegate.h"
 #include "net/base/upload_data_stream.h"
 #include "net/disk_cache/disk_cache.h"
 #include "net/http/disk_based_cert_cache.h"
 #include "net/http/disk_cache_based_quic_server_info.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"
@@ skipping 241 matching lines @@
   virtual QuicServerInfo* GetForServer(
       const QuicServerId& server_id) OVERRIDE {
     return new DiskCacheBasedQuicServerInfo(server_id, http_cache_);
   }
 
  private:
   HttpCache* const http_cache_;
 };
 
 //-----------------------------------------------------------------------------
+
+class HttpCache::AsyncValidation {
+ public:
+  AsyncValidation(const HttpRequestInfo& original_request, HttpCache* cache)
+      : request_(original_request), cache_(cache) {}
+  ~AsyncValidation() {}
+
+  void Start(const BoundNetLog& net_log, NetworkDelegate* network_delegate);
+
+ private:
+  void OnStarted(int result);
+  void DoRead();
+  void OnRead(int result);
+
+  // Terminate this request with net error code |result|. Logs the transaction
+  // result and asks HttpCache to delete this object.
+  // If there was a client or server certificate error, it cannot be recovered
+  // asynchronously, so we need to prevent futures attempts to asynchronously
+  // fetch the resource. In this case, the cache entry is doomed.
+  void Terminate(int result);
+
+  HttpRequestInfo request_;
+  scoped_refptr<IOBuffer> buf_;
+  CompletionCallback read_callback_;
+  scoped_ptr<Transaction> transaction_;
+  base::Time start_time_;
+
+  // The HttpCache object owns this object. This object is always deleted before
+  // the pointer to the cache becomes invalid.
+  HttpCache* cache_;
+
+  DISALLOW_COPY_AND_ASSIGN(AsyncValidation);
+};
+
+void HttpCache::AsyncValidation::Start(const BoundNetLog& net_log,
+                                       NetworkDelegate* network_delegate) {
+  transaction_.reset(new Transaction(IDLE, cache_));

[rvargas (doing something else), 2014/08/29 23:36:06]

It's better to keep transaction creation centralized in CreateTransaction

[Adam Rice, 2014/09/01 14:42:59]

Done.

+  if (network_delegate) {
+    // This code is necessary to enable async transactions to pass over the
+    // data-reduction proxy. This is a violation of the "once-and-only-once"
+    // principle, since it copies code from URLRequestHttpJob. We cannot use the
+    // original callback passed to HttpCache::Transaction by URLRequestHttpJob
+    // as it will only be valid as long as the URLRequestHttpJob object is
+    // alive, and that object will be deleted as soon as the synchronous request
+    // completes.
+    //
+    // This code is also an encapsulation violation. We are exploiting the fact
+    // that the |request| parameter to NotifyBeforeSendProxyHeaders() is never
+    // actually used for anything, and so can be NULL.
+    //
+    // TODO(ricea): Do this better.
+    transaction_->SetBeforeProxyHeadersSentCallback(
+        base::Bind(&NetworkDelegate::NotifyBeforeSendProxyHeaders,
+                   base::Unretained(network_delegate),
+                   static_cast<URLRequest*>(NULL)));
+    // The above use of base::Unretained is safe because the NetworkDelegate has
+    // to live at least as long as the HttpNetworkSession which has to live as
+    // least as long as the HttpNetworkLayer which has to live at least as long
+    // this HttpCache object.
+  }
+
+  transaction_->ValidateInternalCache();

[rvargas (doing something else), 2014/08/29 23:36:06]

This looks out of the blue

I just found the mention of it one the design doc. I think things should go
through standard interfaces, and that means load flags. The code on SetRequest
is very surprising because of this line.

(forcing upstream caches to revalidate sounds ok)

[Adam Rice, 2014/09/01 14:43:00]

I originally was using load flags, but it made me uncomfortable that it set
Cache-Control: max-age=0. The more I've thought about it, the more I've become
convinced that the asynchronous request should be the same as the synchronous
request it replaces.

In particular, servers behind reverse proxies may incur considerable extra cost
when max-age=0 is set on the request, to the point where it may be uneconomical
for them to use stale-while-revalidate at all.

That's not to say that I think this is a wonderful API. In the future I envision
using something like a LOAD_ASYNC_REVALIDATION flag to make sure we don't
re-enter the async pathway. But I already knew I was going to throw away this
implementation when I added this.

Other than adding a new load flag, I can't see a way to do this with existing
interfaces without side-effects.

[rvargas (doing something else), 2014/09/03 01:25:53]

I think it is interesting to bring this up. Basically you are saying that an
user hitting ctrl-f5 causes a very big burden on intermediary proxies to the
point that it changes the economics of the lifetime for a resource. When a user
hits ctrl-F5, we say that whatever we are showing may be out of date and we
force all upstream proxies to revalidate their content with the server. In this
case we are also saying that we believe we may have outdated resources and we
want to double check them with the origin server. It sounds very similar, but I
see your point. Assuming _all_ proxies in the middle understand SWR, an extra
header should not be needed.

From an implementation point of view, I'd prefer to define an extra load flag
now and not have a sidechannel to the cache transaction.

[Adam Rice, 2014/09/05 15:17:11]

Okay, I have added LOAD_ASYNC_REVALIDATION and removed the extra API.

+  start_time_ = base::Time::Now();
+  // This use of base::Unretained is safe because |transaction_| is owned by
+  // this object.
+  read_callback_ = base::Bind(&AsyncValidation::OnRead, base::Unretained(this));
+  // This use of base::Unretained is safe as above.
+  int rv = transaction_->Start(
+      &request_,
+      base::Bind(&AsyncValidation::OnStarted, base::Unretained(this)),
+      net_log);
+
+  if (rv == ERR_IO_PENDING)
+    return;
+
+  OnStarted(rv);
+}
+
+void HttpCache::AsyncValidation::OnStarted(int result) {
+  if (result != OK) {
+    DVLOG(1) << "Asynchronous transaction start failed for " << request_.url;
+    Terminate(result);
+    return;
+  }
+
+  DoRead();
+}
+
+void HttpCache::AsyncValidation::DoRead() {
+  const size_t kBufSize = 4096;
+  if (!buf_)
+    buf_ = new IOBuffer(kBufSize);
+
+  int rv = 0;
+  do {
+    rv = transaction_->Read(buf_.get(), kBufSize, read_callback_);
+  } while (rv > 0);
+
+  if (rv == ERR_IO_PENDING)
+    return;
+
+  OnRead(rv);
+}
+
+void HttpCache::AsyncValidation::OnRead(int result) {
+  if (result > 0) {
+    DoRead();
+    return;
+  }
+  Terminate(result);
+}
+
+void HttpCache::AsyncValidation::Terminate(int result) {
+  if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED || IsCertificateError(result))
+    cache_->DoomEntry(transaction_->key(), transaction_.get());

[rvargas (doing something else), 2014/08/29 23:36:06]

why? Looks like a problem with this code, not with the entry.

[Adam Rice, 2014/09/01 14:43:00]

Consider the case where the remote server suddenly asks for a client
certificate. With this design, there's no way we can satisfy the request.

However, even in a different design where SSL certificate errors can be handled,
we shouldn't pop-up UI for an asynchronous request. So if we need to ask the
user to select a client certificate, we can't do it now, and the request should
fail.

The ideal behaviour is to make sure that if the user hits the same resource
again, the next time we make a synchronous request.

This could maybe be done by adding a flag to HttpResponseInfo, but for the sake
of simplicity this initial implementation just dooms the entry.

[rvargas (doing something else), 2014/09/03 01:25:53]

We don't have to pop up an auth dialog for every resource that is loaded from a
given domain. The auth layer deals with that. I'm quite wary of deleting entries
because we don't have the right code in place... so either the code uses other
code at the 'right' layer or it replicates the required logic.

Even on the final design, if an auth dialog is to be triggered with an async
request because no resource was required from the server before, I would not be
very fond of deleting entries from the cache... even if that means that the
resource is not validated against the server and we have to wait until some auth
credentials are in place.

[Adam Rice, 2014/09/05 15:17:11]

For the final design, I'm thinking of something like having an
async_revalidation_failed flag in HttpResponseInfo that would prevent entering
the async path for the same resource again. But that would mean a disk cache
format change, so it's not something I'd do lightly.

[rvargas (doing something else), 2014/09/05 22:52:39]

My suggestion for the final design would be that the revalidation would not be
initiated by the cache, so whoever takes care of driving the revalidation should
decide what to do.

As for this version, I'm still not convinced of deleting the entry. At least not
without first trying to solve the problem.

[Adam Rice, 2014/09/09 12:36:45]

The fact that another asynchronous validation should not be attempted will still
need to be stored somewhere, and the cache seems like the natural place.

I will try to wire up the certificate-error handling code to AsyncValidation but
it will be ugly and everyone will hate it so I will do it an a separate CL.

+  base::TimeDelta time_saved = base::Time::Now() - start_time_;
+  UMA_HISTOGRAM_TIMES("HttpCache.AsyncValidationTimeSaved", time_saved);

[rvargas (doing something else), 2014/08/29 23:36:06]

What is this measuring? This cannot be accounted as the PLT time that was saved
by using this feature, right?

[Adam Rice, 2014/09/01 14:43:00]

Sadly, no.

I was thinking of splitting it into two histograms: CSS/sync JS/fonts--"things
that are in the critical path to display the page" and "other". But it appears
we can't distinguish sync JS from async JS at this layer. Also, even things in
the critical path can be loaded in parallel, so it wouldn't be all that
meaningful.

It's more of an "AsyncValidationDidSomething" metric than anything else. I can't
think of a better name for it right now.

[rvargas (doing something else), 2014/09/03 01:25:53]

I'm fine with it as long as we don't use as a metric for the success of the
experiment.

[Adam Rice, 2014/09/05 15:17:11]

I renamed it AsyncValidationDuration to avoid misunderstandings.

+  transaction_->net_log().EndEventWithNetErrorCode(
+      NetLog::TYPE_ASYNC_REVALIDATION, result);
+  cache_->DeleteAsyncValidation(this);
+  // |this| is deleted.
+}
+
+//-----------------------------------------------------------------------------
 HttpCache::HttpCache(const net::HttpNetworkSession::Params& params,
                      BackendFactory* backend_factory)
     : net_log_(params.net_log),
       backend_factory_(backend_factory),
       building_backend_(false),
       bypass_lock_for_test_(false),
+      use_stale_while_revalidate_(params.use_stale_while_revalidate),
       mode_(NORMAL),
       network_layer_(new HttpNetworkLayer(new HttpNetworkSession(params))),
       weak_factory_(this) {
   SetupQuicServerInfoFactory(network_layer_->GetSession());
 }
 
 
 // This call doesn't change the shared |session|'s QuicServerInfoFactory because
 // |session| is shared.
 HttpCache::HttpCache(HttpNetworkSession* session,
                      BackendFactory* backend_factory)
     : net_log_(session->net_log()),
       backend_factory_(backend_factory),
       building_backend_(false),
       bypass_lock_for_test_(false),
+      use_stale_while_revalidate_(session->params().use_stale_while_revalidate),
       mode_(NORMAL),
       network_layer_(new HttpNetworkLayer(session)),
       weak_factory_(this) {
 }
 
 HttpCache::HttpCache(HttpTransactionFactory* network_layer,
                      NetLog* net_log,
                      BackendFactory* backend_factory)
     : net_log_(net_log),
       backend_factory_(backend_factory),
       building_backend_(false),
       bypass_lock_for_test_(false),
+      use_stale_while_revalidate_(false),
       mode_(NORMAL),
       network_layer_(network_layer),
       weak_factory_(this) {
   SetupQuicServerInfoFactory(network_layer_->GetSession());
+  HttpNetworkSession* session = network_layer_->GetSession();
+  if (session)
+    use_stale_while_revalidate_ = session->params().use_stale_while_revalidate;
 }
 
 HttpCache::~HttpCache() {
   // Transactions should see an invalid cache after this point; otherwise they
   // could see an inconsistent object (half destroyed).
   weak_factory_.InvalidateWeakPtrs();
 
   // If we have any active entries remaining, then we need to deactivate them.
   // We may have some pending calls to OnProcessPendingQueue, but since those
   // won't run (due to our destruction), we can simply ignore the corresponding
   // will_process_pending_queue flag.
   while (!active_entries_.empty()) {
     ActiveEntry* entry = active_entries_.begin()->second;
     entry->will_process_pending_queue = false;
     entry->pending_queue.clear();
     entry->readers.clear();
     entry->writer = NULL;
     DeactivateEntry(entry);
   }
 
   STLDeleteElements(&doomed_entries_);
+  STLDeleteElements(&async_validations_);
 
   // Before deleting pending_ops_, we have to make sure that the disk cache is
   // done with said operations, or it will attempt to use deleted data.
   cert_cache_.reset();
   disk_cache_.reset();
 
   PendingOpsMap::iterator pending_it = pending_ops_.begin();
   for (; pending_it != pending_ops_.end(); ++pending_it) {
     // We are not notifying the transactions about the cache going away, even
     // though they are waiting for a callback that will never fire.
@@ skipping 668 matching lines @@
   // not delete the entry before OnProcessPendingQueue runs.
   if (entry->will_process_pending_queue)
     return;
   entry->will_process_pending_queue = true;
 
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&HttpCache::OnProcessPendingQueue, GetWeakPtr(), entry));
 }
 
+void HttpCache::PerformAsyncValidation(const HttpRequestInfo& original_request,
+                                       const BoundNetLog& net_log) {
+  DCHECK(use_stale_while_revalidate_);
+  AsyncValidation* job = new AsyncValidation(original_request, this);
+  bool insert_ok = async_validations_.insert(job).second;
+  DCHECK(insert_ok);

[rvargas (doing something else), 2014/08/29 23:36:06]

why can't this fail?

[Adam Rice, 2014/09/01 14:43:00]

It is matched with the erase() in DeleteAsyncValidation. new AsyncValidation()
cannot return the same pointer until the old one is freed, and AsyncValidations
are only freed in DeleteAsyncValidation().

Strictly speaking, they are also freed in the ~HttpCache() destructor, but that
calls InvalidateWeakPtrs() first, ensuring that PerformAsyncValidation() cannot
be called.

[rvargas (doing something else), 2014/09/03 01:25:52]

But we should not be performing two async validations for the same resource at
the same time, right? Shouldn't we ignore a second request if a another is in
progress?

[Adam Rice, 2014/09/05 15:17:11]

It might be possible to get multiple AsyncValidations to the same URL due to a
race condition, but I haven't managed to get it to happen in my tests. All that
will happen is that they will all wait behind the cache lock for the first one
to complete, and then (assuming the revalidation was successful) complete from
the cache.

Anyway, the std::set is only comparing the pointers, it doesn't care whether
they refer to the same resource or not.

[rvargas (doing something else), 2014/09/05 22:52:39]

Sorry I was not clear. Yes, things will be queued by the lock (so there is no
race), but that means consecutive revalidations are going through the wire.
Clearly, the first revalidation should invalidate the others as the assumption
that a revalidation is needed doesn't hold anymore.

So while I see why a DCHECK here (with the current implementation) should never
fire (and it is a good invariant check), I think we should be keeping track of
the URL being revalidated and not queue another revalidation for the same
resource.

[Adam Rice, 2014/09/09 12:36:45]

Done.

+  HttpNetworkSession* network_session = GetSession();
+  NetworkDelegate* network_delegate = NULL;
+  if (network_session)
+    network_delegate = network_session->network_delegate();
+  job->Start(net_log, network_delegate);

[rvargas (doing something else), 2014/08/29 23:36:06]

AsyncValidation is not a UrlRequest::Job so it's better to use another name
here.

[Adam Rice, 2014/09/01 14:43:00]

Done.

+  // |job| may have been deleted here.
+}
+
+void HttpCache::DeleteAsyncValidation(AsyncValidation* async_validation) {
+  size_t erased = async_validations_.erase(async_validation);
+  DCHECK_EQ(1U, erased);
+  delete async_validation;
+}
+
 void HttpCache::OnProcessPendingQueue(ActiveEntry* entry) {
   entry->will_process_pending_queue = false;
   DCHECK(!entry->writer);
 
   // If no one is interested in this entry, then we can deactivate it.
   if (entry->pending_queue.empty()) {
     if (entry->readers.empty())
       DestroyEntry(entry);
     return;
   }
@@ skipping 150 matching lines @@
     building_backend_ = false;
     DeletePendingOp(pending_op);
   }
 
   // The cache may be gone when we return from the callback.
   if (!item->DoCallback(result, disk_cache_.get()))
     item->NotifyTransaction(result, NULL);
 }
 
 }  // namespace net