Cronet: Use stale DNS cache entries experimentally.

net/dns/stale_host_resolver.cc

Index: net/dns/stale_host_resolver.cc
diff --git a/net/dns/stale_host_resolver.cc b/net/dns/stale_host_resolver.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ece47352e1803606d88d9ccdc6af96b705877f39
--- /dev/null
+++ b/net/dns/stale_host_resolver.cc
@@ -0,0 +1,347 @@
+// 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/dns/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"
+
+#define STALE_HISTOGRAM_ENUM(name, value, max) \
+  UMA_HISTOGRAM_ENUMERATION("DNS.StaleHostResolver." name, value, max)
+
+#define STALE_HISTOGRAM_TIME(name, time) \
+  UMA_HISTOGRAM_MEDIUM_TIMES("DNS.StaleHostResolver." name, time)
+
+namespace net {
+
+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 HistogramRequestOutcome(RequestOutcome outcome) {
+  STALE_HISTOGRAM_ENUM("RequestOutcome", outcome, MAX_REQUEST_OUTCOME);
+}
+
+void HistogramAddressListDelta(AddressListDeltaType delta) {
+  STALE_HISTOGRAM_ENUM("StaleAddressListDelta", delta, MAX_DELTA_TYPE);
+}
+
+void HistogramTimeDelta(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.

[Randy Smith (Not in Mondays), 2016/05/04 21:00:28]

Please document the ownership and lifetime model for Request, especially since
at first glance it seems a little weird :-}.  (I'd feel a bit more comfortable
if it was owned and deleted by StaleHostResolver, instead of self-owned, but
that's just because it's a model I'm more used to.)

[Julia Tuttle, 2016/05/11 20:35:10]

It is a little of both :)

Request is self-owned, but StaleHostResolver will cancel pending jobs at
destruction.

(See the creation of |wrapped_callback| in Resolve -- that's how
StaleHostResolver keeps track of pending jobs.)

+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 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.

[Randy Smith (Not in Mondays), 2016/05/04 21:00:28]

I'm assuming that this is the standard "return OK+set *addresses || return
ERR_IO_PENDING -> calling callback, having filled in *addresses || return other
net error code"; that right?  I have a slight preference for documenting that
even if it is the standard, but I'm willing to leave that up to you (internal
class in .cc file and all that).

[Julia Tuttle, 2016/05/11 20:35:10]

Done.

+  int Start(HostResolver* resolver,
+            const RequestInfo& info,
+            RequestPriority priority,
+            AddressList* addresses,

[Randy Smith (Not in Mondays), 2016/05/04 21:00:28]

nit: Could you document that the caller must guarantee that |*addresses| remains
valid for the lifetime of the Request?

[Julia Tuttle, 2016/05/11 20:35:10]

Done.

+            const CompletionCallback& callback,
+            const 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(HostResolver* resolver);
+
+  bool has_network_request() const { return !!network_handle_; }
+  bool returned_stale_result() const { return callback_.is_null(); }
+
+ private:
+  ~Request();
+
+  // 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 AddressList& addresses);
+
+  void HistogramSynchronousRequest();

[Randy Smith (Not in Mondays), 2016/05/04 21:00:28]

nit, suggestion: Change these names to match the other CL, i.e. using "Record"
as the verb rather than "Histogram"?

[Julia Tuttle, 2016/05/11 20:35:09]

Done.

+  void HistogramNetworkRequest(int error);
+  void HistogramCanceledRequest();
+
+  AddressList* addresses_;
+  CompletionCallback callback_;
+
+  int stale_error_;
+  AddressList stale_addresses_;
+  base::OneShotTimer stale_timer_;
+
+  AddressList network_addresses_;
+  HostResolver::RequestHandle network_handle_;
+};
+
+StaleHostResolver::Request::Request()
+    : addresses_(nullptr), network_handle_(nullptr) {}
+
+int StaleHostResolver::Request::Start(
+    HostResolver* resolver,
+    const RequestInfo& info,
+    RequestPriority priority,
+    AddressList* addresses,
+    const CompletionCallback& callback,
+    const BoundNetLog& net_log,
+    const StaleEntryUsableCallback& usable_callback,
+    base::TimeDelta stale_delay) {
+  DCHECK(!callback.is_null());
+  DCHECK(!usable_callback.is_null());
+
+  callback_ = callback;
+  addresses_ = addresses;
+
+  CompletionCallback network_callback =
+      base::Bind(&StaleHostResolver::Request::OnNetworkRequestComplete,
+                 base::Unretained(this));

[Randy Smith (Not in Mondays), 2016/05/04 21:00:28]

What guarantees that the Request will stick around until the network_callback
returns?  I don't see anything canceling the request at the host resolver level
in the destructor.  (There are totally other ways to do it, I just pull towards
that one since it keeps the logic all in the Request class.)

+  base::Callback<void()> stale_callback = base::Bind(
+      &StaleHostResolver::Request::OnStaleDelayElapsed, base::Unretained(this));

[Randy Smith (Not in Mondays), 2016/05/04 21:00:28]

nit, suggestion: If you don't bother with a local variable for this and instead
just include the bind in the argument to stale_timer_.Start(), you implicit
answer the question around why base::Unretained() is safe.  Otherwise, I might
request a comment :-}.

[Julia Tuttle, 2016/05/11 20:35:10]

Done.

+
+  HostCache::EntryStaleness stale_info;
+  int rv = resolver->ResolveStale(
+      info, priority, &network_addresses_, network_callback, &network_handle_,
+      &stale_error_, &stale_addresses_, &stale_info, net_log);
+  if (rv != ERR_IO_PENDING) {
+    DCHECK(!network_handle_);
+    rv = HandleResult(rv, network_addresses_);
+    HistogramSynchronousRequest();
+    delete this;
+    return rv;
+  }
+
+  if (stale_error_ != ERR_DNS_CACHE_MISS && usable_callback.Run(stale_info))
+    stale_timer_.Start(FROM_HERE, stale_delay, stale_callback);
+  else
+    stale_addresses_.clear();
+
+  return ERR_IO_PENDING;
+}
+
+void StaleHostResolver::Request::Cancel(HostResolver* resolver) {
+  DCHECK(has_network_request());
+  DCHECK(!returned_stale_result());
+
+  resolver->CancelRequest(network_handle_);
+  network_handle_ = nullptr;
+
+  HistogramCanceledRequest();
+  delete this;
+}
+
+StaleHostResolver::Request::~Request() {
+  DCHECK(!has_network_request());
+}
+
+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_));
+
+  HistogramNetworkRequest(error);
+  delete this;
+}
+
+int StaleHostResolver::Request::HandleResult(int rv,
+                                             const AddressList& addresses) {
+  DCHECK(addresses_);
+
+  if (rv == OK)
+    *addresses_ = addresses;
+  addresses_ = nullptr;
+  return rv;
+}
+
+void StaleHostResolver::Request::HistogramSynchronousRequest() {
+  HistogramRequestOutcome(SYNCHRONOUS);
+}
+
+void StaleHostResolver::Request::HistogramNetworkRequest(int error) {
+  if (stale_timer_.IsRunning() || returned_stale_result())
+    HistogramTimeDelta(base::TimeTicks::Now(), stale_timer_.desired_run_time());
+
+  if (returned_stale_result() && stale_error_ == OK && error == OK) {
+    HistogramAddressListDelta(
+        FindAddressListDeltaType(stale_addresses_, network_addresses_));
+  }
+
+  if (returned_stale_result())
+    HistogramRequestOutcome(STALE_BEFORE_NETWORK);
+  else if (stale_timer_.IsRunning())
+    HistogramRequestOutcome(NETWORK_WITH_STALE);
+  else
+    HistogramRequestOutcome(NETWORK_WITHOUT_STALE);
+}
+
+void StaleHostResolver::Request::HistogramCanceledRequest() {
+  if (stale_timer_.IsRunning())
+    HistogramRequestOutcome(CANCELED_WITH_STALE);
+  else
+    HistogramRequestOutcome(CANCELED_WITHOUT_STALE);
+}
+
+namespace {
+
+bool StaleEntryIsUsable(const StaleHostResolver::StaleOptions& options,
+                        const 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
+
+StaleHostResolver::StaleHostResolver(
+    std::unique_ptr<HostResolver> inner_resolver,
+    const StaleOptions& stale_options)
+    : resolver_(std::move(inner_resolver)), options_(stale_options) {
+  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,
+                               RequestPriority priority,
+                               AddressList* addresses,
+                               const CompletionCallback& callback,
+                               RequestHandle* out_req,
+                               const BoundNetLog& net_log) {
+  StaleHostResolver::Request::StaleEntryUsableCallback usable_callback =
+      base::Bind(&StaleEntryIsUsable, options_);
+
+  Request* request = new Request();
+  pending_requests_.insert(request);
+  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 == ERR_IO_PENDING && out_req)
+    *out_req = reinterpret_cast<RequestHandle>(request);
+  return rv;

[Randy Smith (Not in Mondays), 2016/05/04 21:00:28]

Doesn't this leak request if request->Start() returns synchronously?

+}
+
+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,
+                                        AddressList* addresses,
+                                        const BoundNetLog& net_log) {
+  return resolver_->ResolveFromCache(info, addresses, net_log);
+}
+
+int StaleHostResolver::ResolveStaleFromCache(
+    const RequestInfo& info,
+    AddressList* addresses,
+    HostCache::EntryStaleness* stale_info,
+    const BoundNetLog& net_log) {
+  return resolver_->ResolveStaleFromCache(info, addresses, stale_info, net_log);
+}
+
+void StaleHostResolver::SetDnsClientEnabled(bool enabled) {
+  resolver_->SetDnsClientEnabled(enabled);
+}
+
+HostCache* StaleHostResolver::GetHostCache() {
+  return resolver_->GetHostCache();
+}
+
+std::unique_ptr<base::Value> StaleHostResolver::GetDnsConfigAsValue() const {
+  return resolver_->GetDnsConfigAsValue();
+}
+
+void StaleHostResolver::OnRequestComplete(
+    StaleHostResolver::Request* request,
+    const CompletionCallback& outer_callback,
+    int error) {
+  DCHECK(request);
+  DCHECK_NE(ERR_IO_PENDING, error);
+
+  DCHECK_EQ(1u, pending_requests_.count(request));
+  pending_requests_.erase(request);
+
+  outer_callback.Run(error);
+}
+
+}  // namespace net