Cronet: Use stale DNS cache entries experimentally.

components/cronet/stale_host_resolver.cc

+// 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 "components/cronet/stale_host_resolver.h"
+
+#include "base/callback_helpers.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/timer/timer.h"
+#include "base/values.h"
+#include "net/base/net_errors.h"
+#include "net/dns/dns_util.h"
+#include "net/dns/host_resolver_impl.h"
+
+#define STALE_HISTOGRAM_ENUM(name, value, max) \

[xunjieli, 2016/06/13 19:25:04]

I am not sure if we want these defines. Can we inline them?

[Julia Tuttle, 2016/06/14 23:11:32]

Done.

+  UMA_HISTOGRAM_ENUMERATION("DNS.StaleHostResolver." name, value, max)
+
+#define STALE_HISTOGRAM_TIME(name, time) \
+  UMA_HISTOGRAM_MEDIUM_TIMES("DNS.StaleHostResolver." name, time)
+
+namespace cronet {
+
+namespace {
+
+// Used in histograms; do not modify existing values.
+enum RequestOutcome {
+  // Served from (valid) cache, hosts file, IP literal, etc.
+  SYNCHRONOUS = 0,
+
+  // Network responded; there was no usable stale data.
+  NETWORK_WITHOUT_STALE = 1,
+
+  // Network responded before stale delay; there was usable stale data.
+  NETWORK_WITH_STALE = 2,
+
+  // Stale data returned; network didn't respond before the stale delay.
+  STALE_BEFORE_NETWORK = 3,
+
+  // Request canceled; there was no usable stale data.
+  CANCELED_WITHOUT_STALE = 4,
+
+  // Request canceled; there was usable stale data.
+  CANCELED_WITH_STALE = 5,
+
+  MAX_REQUEST_OUTCOME
+};
+
+void RecordRequestOutcome(RequestOutcome outcome) {
+  STALE_HISTOGRAM_ENUM("RequestOutcome", outcome, MAX_REQUEST_OUTCOME);
+}
+
+void RecordAddressListDelta(net::AddressListDeltaType delta) {
+  STALE_HISTOGRAM_ENUM("StaleAddressListDelta", delta, net::MAX_DELTA_TYPE);
+}
+
+void RecordTimeDelta(base::TimeTicks network_time, base::TimeTicks stale_time) {
+  if (network_time < stale_time)
+    STALE_HISTOGRAM_TIME("NetworkEarly", stale_time - network_time);
+  else
+    STALE_HISTOGRAM_TIME("NetworkLate", network_time - stale_time);
+}
+
+}  // namespace
+
+// A request made by the StaleHostResolver. May return fresh cached data,
+// network data, or stale cached data.
+class StaleHostResolver::Request {
+ public:
+  Request();
+
+  // A callback for the caller to decide whether a stale entry is usable or not.
+  typedef base::Callback<bool(const net::HostCache::EntryStaleness&)>
+      StaleEntryUsableCallback;
+
+  // Starts the request. May call |usable_callback| inline if |resolver| returns
+  // stale data to let the caller decide whether the data is usable.
+  //
+  // Returns the result if the request finishes synchronously. Returns
+  // ERR_IO_PENDING and calls |callback| with the result if it finishes
+  // asynchronously, unless the requets is canceled (via |Cancel()|).

[xunjieli, 2016/06/13 19:25:04]

typo in "request"

[Julia Tuttle, 2016/06/14 23:11:32]

Done.

+  //
+  // |addresses| must remain valid until the Request completes (synchronously or
+  // via |callback|) or is canceled (via |Cancel()|).
+  int Start(net::HostResolverImpl* resolver,
+            const RequestInfo& info,
+            net::RequestPriority priority,
+            net::AddressList* addresses,
+            const net::CompletionCallback& callback,
+            const net::BoundNetLog& net_log,
+            const StaleEntryUsableCallback& usable_callback,
+            base::TimeDelta stale_delay);
+
+  // Cancels the request. |resolver| must be the same resolver passed to
+  // |Start()|. Does not call the callback passed to |Start()|.
+  //
+  // |Start()| must have been called and returned ERR_IO_PENDING; the request
+  // must not have been canceled already.
+  void Cancel(net::HostResolverImpl* resolver);
+
+  bool has_network_request() const { return network_handle_ != nullptr; }
+  bool returned_stale_result() const { return callback_.is_null(); }
+
+ private:
+  ~Request();
+
+  static int ResolveStale(net::HostResolverImpl* resolver,

[xunjieli, 2016/06/13 19:25:04]

Need documentation, specifically, what the return value is.

[Julia Tuttle, 2016/06/14 23:11:32]

Done.

+                          const net::HostResolver::RequestInfo& info,
+                          net::RequestPriority priority,
+                          net::AddressList* addresses,
+                          const net::CompletionCallback& callback,
+                          net::HostResolver::RequestHandle* out_req,
+                          int* stale_error,
+                          net::AddressList* stale_addresses,
+                          net::HostCache::EntryStaleness* stale_info,
+                          const net::BoundNetLog& net_log);
+
+  // Callback for the timer to return stale data.
+  void OnStaleDelayElapsed();
+
+  // Callback for the underlying network request.
+  void OnNetworkRequestComplete(int error);
+
+  // Populates |*addresses_| from |addresses| if and only if rv is OK, clears
+  // |addresses_| to make sure the request doesn't return twice, and then
+  // returns |rv|.
+  int HandleResult(int rv, const net::AddressList& addresses);
+
+  void RecordSynchronousRequest();
+  void RecordNetworkRequest(int error);
+  void RecordCanceledRequest();
+
+  net::AddressList* addresses_;
+  net::CompletionCallback callback_;
+
+  int stale_error_;
+  net::AddressList stale_addresses_;
+  base::OneShotTimer stale_timer_;
+
+  net::AddressList network_addresses_;
+  net::HostResolver::RequestHandle network_handle_;

[xunjieli, 2016/06/13 19:25:03]

need documentation on these member variables.

[Julia Tuttle, 2016/06/14 23:11:32]

Done.

+};
+
+StaleHostResolver::Request::Request()
+    : addresses_(nullptr), network_handle_(nullptr) {}
+
+int StaleHostResolver::Request::Start(
+    net::HostResolverImpl* resolver,
+    const RequestInfo& info,
+    net::RequestPriority priority,
+    net::AddressList* addresses,
+    const net::CompletionCallback& callback,
+    const net::BoundNetLog& net_log,
+    const StaleEntryUsableCallback& usable_callback,
+    base::TimeDelta stale_delay) {
+  DCHECK(!callback.is_null());
+  DCHECK(!usable_callback.is_null());
+
+  callback_ = callback;
+  addresses_ = addresses;
+
+  // Request never deletes itself until the network request returns (if one is
+  // started), so it's safe to use Unretained here.
+  net::CompletionCallback network_callback =
+      base::Bind(&StaleHostResolver::Request::OnNetworkRequestComplete,
+                 base::Unretained(this));
+  net::HostCache::EntryStaleness stale_info;
+  int rv = ResolveStale(resolver, info, priority, &network_addresses_,
+                        network_callback, &network_handle_, &stale_error_,
+                        &stale_addresses_, &stale_info, net_log);
+  if (rv != net::ERR_IO_PENDING) {
+    DCHECK(!network_handle_);
+    rv = HandleResult(rv, network_addresses_);
+    RecordSynchronousRequest();
+    delete this;
+    return rv;
+  }
+
+  if (stale_error_ != net::ERR_DNS_CACHE_MISS &&
+      usable_callback.Run(stale_info)) {
+    // |stale_timer_| is deleted when the Request is deleted, so it's safe to
+    // use Unretained here.
+    base::Callback<void()> stale_callback =
+        base::Bind(&StaleHostResolver::Request::OnStaleDelayElapsed,
+                   base::Unretained(this));
+    stale_timer_.Start(FROM_HERE, stale_delay, stale_callback);
+  } else {
+    stale_addresses_.clear();
+  }
+
+  return net::ERR_IO_PENDING;
+}
+
+void StaleHostResolver::Request::Cancel(net::HostResolverImpl* resolver) {
+  DCHECK(has_network_request());
+  DCHECK(!returned_stale_result());
+
+  resolver->CancelRequest(network_handle_);
+  network_handle_ = nullptr;
+
+  RecordCanceledRequest();
+  delete this;
+}
+
+StaleHostResolver::Request::~Request() {
+  DCHECK(!has_network_request());
+}
+
+// static
+int StaleHostResolver::Request::ResolveStale(
+    net::HostResolverImpl* resolver,
+    const net::HostResolver::RequestInfo& info,
+    net::RequestPriority priority,
+    net::AddressList* addresses,
+    const net::CompletionCallback& callback,
+    net::HostResolver::RequestHandle* out_req,
+    int* stale_error,
+    net::AddressList* stale_addresses,
+    net::HostCache::EntryStaleness* stale_info,
+    const net::BoundNetLog& net_log) {
+  net::AddressList cache_addresses;
+  int cache_rv = resolver->ResolveStaleFromCache(info, &cache_addresses,
+                                                 stale_info, net_log);
+  // If it's a fresh cache hit (or literal), return it synchronously.
+  if (cache_rv != net::ERR_DNS_CACHE_MISS && !stale_info->is_stale()) {
+    *stale_error = net::ERR_UNEXPECTED;
+    *addresses = cache_addresses;
+    return cache_rv;
+  }
+
+  // If it's a stale cache hit, fill in |*stale_addresses| but run the network
+  // request anyway.
+  if (cache_rv != net::ERR_DNS_CACHE_MISS)
+    *stale_addresses = cache_addresses;
+
+  *stale_error = cache_rv;
+
+  // Don't check the cache again.
+  net::HostResolver::RequestInfo no_cache_info(info);
+  no_cache_info.set_allow_cached_response(false);
+  return resolver->Resolve(no_cache_info, priority, addresses, callback,
+                           out_req, net_log);
+}
+
+void StaleHostResolver::Request::OnStaleDelayElapsed() {
+  DCHECK(has_network_request());
+  DCHECK(!returned_stale_result());
+
+  base::ResetAndReturn(&callback_)
+      .Run(HandleResult(stale_error_, stale_addresses_));
+
+  // Don't delete |this|; let the underlying request continue to backfill the
+  // host cache.
+}
+
+void StaleHostResolver::Request::OnNetworkRequestComplete(int error) {
+  DCHECK(has_network_request());
+
+  network_handle_ = nullptr;
+
+  if (!returned_stale_result())
+    callback_.Run(HandleResult(error, network_addresses_));
+
+  RecordNetworkRequest(error);
+  delete this;
+}
+
+int StaleHostResolver::Request::HandleResult(
+    int rv,
+    const net::AddressList& addresses) {
+  DCHECK(addresses_);
+
+  if (rv == net::OK)
+    *addresses_ = addresses;
+  addresses_ = nullptr;
+  return rv;
+}
+
+void StaleHostResolver::Request::RecordSynchronousRequest() {
+  RecordRequestOutcome(SYNCHRONOUS);
+}
+
+void StaleHostResolver::Request::RecordNetworkRequest(int error) {
+  if (stale_timer_.IsRunning() || returned_stale_result())
+    RecordTimeDelta(base::TimeTicks::Now(), stale_timer_.desired_run_time());
+
+  if (returned_stale_result() && stale_error_ == net::OK && error == net::OK) {
+    RecordAddressListDelta(
+        FindAddressListDeltaType(stale_addresses_, network_addresses_));
+  }
+
+  if (returned_stale_result())
+    RecordRequestOutcome(STALE_BEFORE_NETWORK);
+  else if (stale_timer_.IsRunning())
+    RecordRequestOutcome(NETWORK_WITH_STALE);
+  else
+    RecordRequestOutcome(NETWORK_WITHOUT_STALE);
+}
+
+void StaleHostResolver::Request::RecordCanceledRequest() {
+  if (stale_timer_.IsRunning())
+    RecordRequestOutcome(CANCELED_WITH_STALE);
+  else
+    RecordRequestOutcome(CANCELED_WITHOUT_STALE);
+}
+
+namespace {
+
+bool StaleEntryIsUsable(const StaleHostResolver::StaleOptions& options,
+                        const net::HostCache::EntryStaleness& entry) {
+  if (options.max_expired_time != base::TimeDelta() &&
+      entry.expired_by > options.max_expired_time) {
+    return false;
+  }
+  if (options.max_stale_uses > 0 && entry.stale_hits > options.max_stale_uses)
+    return false;
+  if (!options.allow_other_network && entry.network_changes > 0)
+    return false;
+  return true;
+}
+
+}  // namespace

[xunjieli, 2016/06/13 19:25:03]

Can we have the anonymous namespaces at the top of the file with the other
anonymous namespace?

[Julia Tuttle, 2016/06/14 23:11:32]

Done.

+
+StaleHostResolver::StaleHostResolver(
+    std::unique_ptr<net::HostResolverImpl> inner_resolver,
+    const StaleOptions& stale_options)
+    : resolver_(std::move(inner_resolver)), options_(stale_options) {

[xunjieli, 2016/06/13 19:25:04]

nit: Can we rename |resolver_| to |inner_resolver_|? I think it is slightly
clearer that way.

[Julia Tuttle, 2016/06/14 23:11:32]

Done.

+  DCHECK_GE(0, stale_options.max_expired_time.InMicroseconds());
+  DCHECK_GE(0, stale_options.max_stale_uses);
+}
+
+StaleHostResolver::~StaleHostResolver() {
+  for (auto* request : pending_requests_)
+    request->Cancel(resolver_.get());
+  pending_requests_.clear();
+}
+
+int StaleHostResolver::Resolve(const RequestInfo& info,
+                               net::RequestPriority priority,
+                               net::AddressList* addresses,
+                               const net::CompletionCallback& callback,
+                               RequestHandle* out_req,
+                               const net::BoundNetLog& net_log) {
+  StaleHostResolver::Request::StaleEntryUsableCallback usable_callback =
+      base::Bind(&StaleEntryIsUsable, options_);
+
+  Request* request = new Request();
+  pending_requests_.insert(request);

[xunjieli, 2016/06/13 19:25:04]

If request->Start() finishes synchronously, can we not insert |request| into
|pending_requests| ? That will remove the need to delete the pointer in
Request::Start().

[Julia Tuttle, 2016/06/14 23:11:32]

Done.

+  // If Request is not complete when the StaleHostResolver is being destroyed,
+  // the destructor will Cancel it, preventing this callback from being called,
+  // so it's safe to use Unretained here.
+  net::CompletionCallback wrapped_callback =
+      base::Bind(&StaleHostResolver::OnRequestComplete, base::Unretained(this),
+                 request, callback);
+
+  int rv = request->Start(resolver_.get(), info, priority, addresses,
+                          wrapped_callback, net_log, usable_callback,
+                          options_.delay);
+  if (rv == net::ERR_IO_PENDING && out_req)
+    *out_req = reinterpret_cast<RequestHandle>(request);
+  return rv;
+}
+
+void StaleHostResolver::CancelRequest(RequestHandle req_handle) {
+  Request* request = reinterpret_cast<Request*>(req_handle);
+  DCHECK(request);
+  DCHECK_EQ(1u, pending_requests_.count(request));
+
+  request->Cancel(resolver_.get());
+  pending_requests_.erase(request);
+}
+
+int StaleHostResolver::ResolveFromCache(const RequestInfo& info,
+                                        net::AddressList* addresses,
+                                        const net::BoundNetLog& net_log) {
+  return resolver_->ResolveFromCache(info, addresses, net_log);
+}
+
+void StaleHostResolver::SetDnsClientEnabled(bool enabled) {
+  resolver_->SetDnsClientEnabled(enabled);
+}
+
+net::HostCache* StaleHostResolver::GetHostCache() {
+  return resolver_->GetHostCache();
+}
+
+std::unique_ptr<base::Value> StaleHostResolver::GetDnsConfigAsValue() const {
+  return resolver_->GetDnsConfigAsValue();
+}
+
+void StaleHostResolver::OnRequestComplete(
+    StaleHostResolver::Request* request,
+    const net::CompletionCallback& outer_callback,
+    int error) {
+  DCHECK(request);
+  DCHECK_NE(net::ERR_IO_PENDING, error);
+
+  DCHECK_EQ(1u, pending_requests_.count(request));
+  pending_requests_.erase(request);

[xunjieli, 2016/06/13 19:25:04]

Instead of deleting |request| in OnNetworkRequestComplete, we can delete
|request| here.  It's easier to reason that if StaleHostResolver manages the
lifetime of the |request| entirely. 

Maybe also get rid of OnStaleDelayElapsed and OnNetworkRequestComplete, and bind
a bool to OnRequestComplete callback to specify whether the callback is invoked
with stale results. 

[Julia Tuttle, 2016/06/14 23:11:32]

Done.

+
+  outer_callback.Run(error);
+}
+
+}  // namespace net