Cronet: Use stale DNS cache entries experimentally.

net/dns/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 "net/dns/stale_host_resolver.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 {

[Randy Smith (Not in Mondays), 2016/04/28 17:48:25]

nit, suggestion (I'll stop making these once you argue with me once :-}): I find
it hard to parse out what comments belong with what enums in this type of
commenting style and would prefer a space between an enum and the next enums
comment.

[Julia Tuttle, 2016/04/29 18:04:29]

Happily done, as I still get my 80 characters to write a description :)

+  // 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) {

[Randy Smith (Not in Mondays), 2016/04/28 17:48:25]

Ah.  Feel free to ignore my comment on the other CL :-}.

[Julia Tuttle, 2016/04/29 18:04:29]

Done.

+  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.
+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::StaleEntryInfo&)>
+      StaleEntryUsableCallback;
+
+  // Starts the request. May call |usable_callback| inline if |resolver| returns

[Randy Smith (Not in Mondays), 2016/04/28 17:48:25]

I'm scared about interfaces that are variations (especially more complex
variations) on the usual netstack "return value in synchronous return, call
callback if async".  There's potential complex re-entry problems with calling
callbacks synchronously (Elly tried for filters, Matt shot her down, I don't
remember the issues) and there's potentially complex race issues if you've got
several callbacks running around.

[Julia Tuttle, 2016/04/29 18:04:29]

|usable_callback| isn't a completion callback; it's a lambda to compare the
staleness to some cutoffs.

|callback| isn't actually called from Start; the comment is wrong. (I fixed it.)

+  // stale data. May call the callback passed into the constructor inline if

[Randy Smith (Not in Mondays), 2016/04/28 17:48:25]

What callback passed into the constructor?

[Julia Tuttle, 2016/04/29 18:04:29]

Done.

+  // |resolver| returns valid data synchronously.
+  int Start(HostResolver* resolver,
+            const RequestInfo& info,
+            RequestPriority priority,
+            AddressList* addresses,
+            const CompletionCallback& callback,
+            const BoundNetLog& net_log,
+            const StaleEntryUsableCallback& usable_callback,

[Randy Smith (Not in Mondays), 2016/04/28 17:48:25]

I'm having a hard time mapping the implementation to the above documentation
around this callback.  It looks like it's used for information, i.e. to return a
bool to determine if a stale entry should be returned.  If that's the case,
could you update the doc above to make it clearer?  (I don't have any objections
to using the callback that way--I thought from the above description that it was
being used to pass results back to the caller, which I have more problems with.)

[Julia Tuttle, 2016/04/29 18:04:29]

Done.

+            base::TimeDelta stale_delay);
+
+  // Cancels the request. |resolver| must be the same resolver passed to
+  // |Start()|. Does not call the callback passed t othe constructor.
+  void Cancel(HostResolver* resolver);
+
+  // Callback for the timer to return stale data.
+  void OnStaleDelayElapsed();

[Ryan Sleevi, 2016/04/29 01:04:09]

Pedantry: Seems like it could be private?

[Julia Tuttle, 2016/04/29 18:04:29]

Done.

+
+  // Callback for the underlying network request.
+  void OnNetworkRequestComplete(int error);

[Ryan Sleevi, 2016/04/29 01:04:09]

Pedantry: Seems like it could be private?

[Julia Tuttle, 2016/04/29 18:04:29]

Done.

+
+  bool has_network_request() const { return !!network_handle_; }
+  bool returned_stale_result() const { return callback_.is_null(); }
+
+ private:
+  ~Request();
+
+  // 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();
+  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) {}
+
+StaleHostResolver::Request::~Request() {
+  DCHECK(!has_network_request());
+}
+
+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));
+  base::Callback<void()> stale_callback = base::Bind(
+      &StaleHostResolver::Request::OnStaleDelayElapsed, base::Unretained(this));
+
+  HostCache::StaleEntryInfo 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;
+}
+
+void StaleHostResolver::Request::OnStaleDelayElapsed() {
+  DCHECK(has_network_request());
+  DCHECK(!returned_stale_result());
+
+  callback_.Run(HandleResult(stale_error_, stale_addresses_));
+  callback_.Reset();

[Ryan Sleevi, 2016/04/29 01:04:08]

Maybe add a note as to why you don't delete this

I was trying to figure it out, but my guess is that by not deleting this, not
deleting this is what we use to continue the network request, with the
expectation that
1) No one will ever call Cancel (since we called HandleResult)
2) Eventually the request will complete

[Julia Tuttle, 2016/04/29 18:04:29]

Done.

+}
+
+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()) {

[Ryan Sleevi, 2016/04/29 01:04:09]

nit: no braces for consistency with the rest of the file

[Julia Tuttle, 2016/04/29 18:04:29]

Done.

+    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::StaleEntryInfo& entry) {
+  if (options.max_expired_time != base::TimeDelta() &&
+      entry.expired_by > options.max_expired_time) {
+    return false;
+  }
+  if (options.max_stale_uses > 0u &&
+      entry.stale_hits > options.max_stale_uses) {
+    return false;
+  }
+  if (!options.allow_other_network && entry.network_changes > 0u)
+    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) {}
+
+StaleHostResolver::~StaleHostResolver() {
+  for (auto request : pending_requests_)

[Ryan Sleevi, 2016/04/29 01:04:09]

Shouldn't this be "auto&" or "auto*" (based on https://chromium-cpp.appspot.com/
- I believe auto*)

[Julia Tuttle, 2016/04/29 18:04:29]

Done.

+    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;
+}
+
+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::StaleEntryInfo* 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