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"
+
+#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)
+
+using net::AddressList;
+using net::AddressListDeltaType;
+using net::BoundNetLog;
+using net::CompletionCallback;
+using net::ERR_DNS_CACHE_MISS;
+using net::ERR_IO_PENDING;
+using net::ERR_UNEXPECTED;
+using net::HostCache;
+using net::HostResolver;
+using net::HostResolverImpl;
+using net::MAX_DELTA_TYPE;
+using net::OK;
+using net::RequestPriority;

[Randy Smith (Not in Mondays), 2016/06/08 19:20:03]

https://google.github.io/styleguide/cppguide.html#Namespaces says "Do not use
using-directives" and my biases agree; we in general accept the cost of longer
lines in order to have all the context for references at the point of coding. 
Having said that, I believe the coding guide reference is mostly about
using-directives that import entire namespaces, and I have see using-directives
elsewhere in the code.  So call this a "strong suggestion"; if you feel
strongly, I'll yield (and as always if you provide a good counter-argument I'll
yield) but I'd rather see these all expanded inline.

[Julia Tuttle, 2016/06/10 17:31:44]

Done, but to me, this is morally part of net/, just something that's only used
by cronet and therefore contained in that directory.

+
+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(AddressListDeltaType delta) {
+  STALE_HISTOGRAM_ENUM("StaleAddressListDelta", delta, 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);
+}
+
+int ResolveStale(HostResolverImpl* resolver,

[Randy Smith (Not in Mondays), 2016/06/08 19:20:03]

nit, suggestion: Since this is a helper routine used later in the file, I could
imagine it would be useful to include a comment describing it's intended use
(interface contract style, like what we do in class decls).  But it's a pretty
simple function, and I think it's only used in one place nearby so it's easy to
use the code as the reference, so up to you.

ETA: Though thinking about it, a simple function that's only used in one place
might be a candidate for hoisting into the caller?  (Again, up to you.)

[Julia Tuttle, 2016/06/10 17:31:44]

I had it in the caller, and it felt a little too cluttered? I wanted to make it
clear what I was doing with the fresh and stale results, respectively, as
opposed to the details of how I'm getting them. I'll move it into Request as a
helper method, so it preserves that cleanliness but doesn't clutter the file.

+                 const HostResolver::RequestInfo& info,
+                 RequestPriority priority,
+                 AddressList* addresses,

[Randy Smith (Not in Mondays), 2016/06/08 19:20:02]

nitty nit: Slight preference for calling this network_addresses rather than
addresses, just to match code elsewhere in the file.

[Julia Tuttle, 2016/06/10 17:31:44]

It's not network addresses if there is a fresh cache entry. Maybe
fresh_addresses?

+                 const CompletionCallback& callback,
+                 HostResolver::RequestHandle* out_req,
+                 int* stale_error,
+                 AddressList* stale_addresses,
+                 HostCache::EntryStaleness* stale_info,
+                 const BoundNetLog& net_log) {
+  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 != ERR_DNS_CACHE_MISS && !stale_info->is_stale()) {
+    *stale_error = 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 != ERR_DNS_CACHE_MISS)
+    *stale_addresses = cache_addresses;
+
+  *stale_error = cache_rv;
+
+  // Don't check the cache again.
+  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);
+}
+
+}  // 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::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()|).
+  //
+  // |addresses| must remain valid until the Request completes (synchronously or
+  // via |callback|) or is canceled (via |Cancel()|).
+  int Start(HostResolverImpl* resolver,
+            const RequestInfo& info,
+            RequestPriority priority,
+            AddressList* addresses,
+            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(HostResolverImpl* 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

[Randy Smith (Not in Mondays), 2016/06/08 19:20:02]

nit: |*addresses_|.

[Julia Tuttle, 2016/06/10 17:31:44]

Done.

+  // |addresses_| to make sure the request doesn't return twice, and then
+  // returns |rv|.
+  int HandleResult(int rv, const AddressList& addresses);
+
+  void RecordSynchronousRequest();
+  void RecordNetworkRequest(int error);
+  void RecordCanceledRequest();
+
+  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(
+    HostResolverImpl* 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/06/08 19:20:03]

semi-nit: Comments above the base::Unretaineds as to why they are safe? 
(Everywhere in the file.)

[Julia Tuttle, 2016/06/10 17:31:44]

Done.

+  base::Callback<void()> stale_callback = base::Bind(
+      &StaleHostResolver::Request::OnStaleDelayElapsed, base::Unretained(this));
+
+  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 != ERR_IO_PENDING) {
+    DCHECK(!network_handle_);
+    rv = HandleResult(rv, network_addresses_);
+    RecordSynchronousRequest();
+    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(HostResolverImpl* resolver) {
+  DCHECK(has_network_request());
+  DCHECK(!returned_stale_result());
+
+  resolver->CancelRequest(network_handle_);
+  network_handle_ = nullptr;
+
+  RecordCanceledRequest();
+  delete this;

[Randy Smith (Not in Mondays), 2016/06/08 19:20:02]

This is unusual enough behavior that it really should be called out in the
interface contract comment in the declaration.

Also, do we really need this behavior?  I consider it sometimes necessary but
always dangerous (you're generally leaving *something* with a pointer to freed
memory when you do this); could we simply have the destructor cancel if
necessary, and have the owning StaleHostResolver destroy on cancel? 
Alternatively, if you want to keep this logic under a routine named "Cancel()"
can you just hoist the destruction up into the caller?

[Julia Tuttle, 2016/06/10 17:31:44]

See below; right now, the caller is *happily* unaware of Requests that are
lingering to let underlying network requests finish. It would complicate
StaleHostResolver if it had to explicitly manage the lifetimes of its Requests.

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

[Randy Smith (Not in Mondays), 2016/06/08 19:20:03]

Hmm.  To me, this comment implies that we *could* delete this here without ill
effects, but I don't think we can, since the StaleHostResolver still has a
pointer to the object?

[Julia Tuttle, 2016/06/10 17:31:44]

Done.

+}
+
+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;

[Randy Smith (Not in Mondays), 2016/06/08 19:20:02]

Um ... this isn't wrong, but it is scary.  The fact that this doesn't cause any
problems has to do with what callback_ is set to (which is something that will
get rid of the owner's pointer to this object without indirecting through it),
and counting on that specific property of callback_ inside a method of a utility
class seems dangerous.  At a minimum, could you document that requirement on
callback_ in the comment about the Start() declaration?  Preferably (yes :-}),
I'd love for you to hoist the deletion up into StaleHostResolver in some
fashion.

[Julia Tuttle, 2016/06/10 17:31:44]

Whoa, hold on, I'm not sure I agree. Indirecting through it is fine; the
callback is run three lines *before* the Request is deleted.

I tried hoisting it up into StaleHostResolver, but then it'd have to do the
juggling of "we have to wait for the network request even if the request returns
stale cached results" instead/as well.

+}
+
+int StaleHostResolver::Request::HandleResult(int rv,
+                                             const AddressList& addresses) {
+  DCHECK(addresses_);
+
+  if (rv == 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_ == OK && error == 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 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<HostResolverImpl> 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;
+}
+
+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);
+}
+
+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