Cronet: Use stale DNS cache entries experimentally.

net/dns/stale_host_resolver.h

Index: net/dns/stale_host_resolver.h
diff --git a/net/dns/stale_host_resolver.h b/net/dns/stale_host_resolver.h
new file mode 100644
index 0000000000000000000000000000000000000000..1c9c3335aff348dad8c1a92d3882e22a6c7bff98
--- /dev/null
+++ b/net/dns/stale_host_resolver.h
@@ -0,0 +1,96 @@
+// 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.
+
+#ifndef NET_DNS_STALE_HOST_RESOLVER_H_
+#define NET_DNS_STALE_HOST_RESOLVER_H_
+
+#include <set>
+
+#include "net/dns/host_resolver.h"
+
+namespace net {
+
+// A HostResolver that wraps another HostResolver that uses it to make requests
+// but "impatiently" returns stale data (if available and usable) after a delay,
+// to reduce DNS latency at the expense of accuracy.
+class NET_EXPORT StaleHostResolver : public HostResolver {
+ public:
+  struct NET_EXPORT StaleOptions {
+    // How long to wait before returning stale data, if available.
+    base::TimeDelta delay;
+
+    // If positive, how long stale data can be past the expiration time before
+    // it's considered unusable. If zero or negative, stale data can be used
+    // indefinitely.
+    base::TimeDelta max_expired_time;
+
+    // If set, stale data from previous networks is usable; if clear, it's not.
+    //
+    // If the other network had a working, correct DNS setup, this can increase
+    // the availability of useful stale results.
+    //
+    // If the other network had a broken (e.g. hijacked for captive portal) DNS
+    // setup, this will instead end up returning useless results.
+    bool allow_other_network;
+
+    // If positive, the maximum number of times a stale entry can be used. If
+    // zero, there is no limit.
+    int max_stale_uses;
+  };
+
+  // Creates a StaleHostResolver that uses |inner_resolver| for actual
+  // resolution, but potentially returns stale data according to
+  // |stale_options|.
+  StaleHostResolver(std::unique_ptr<HostResolver> inner_resolver,
+                    const StaleOptions& stale_options);
+
+  ~StaleHostResolver() override;
+
+  // Resolves as a regular HostResolver, but if stale data is available and
+  // usable (according to the options passed to the constructor), and fresh data
+  // is not returned before the specified delay, returns the stale data instead.
+  //
+  // If stale data is returned, the StaleHostResolver allows the underlying
+  // request to continue in order to repopulate the cache.
+  int Resolve(const RequestInfo& info,
+              RequestPriority priority,
+              AddressList* addresses,
+              const CompletionCallback& callback,
+              RequestHandle* out_req,
+              const BoundNetLog& net_log) override;
+
+  // Cancels a request created by |Resolve|.
+  //
+  // It is not valid to cancel a request once it has returned a result, even if
+  // the result was stale data and the underlying request on the inner resolver
+  // is still running.
+  void CancelRequest(RequestHandle req) override;
+
+  // The remaining public methods pass through to the inner resolver:

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

So there's a code smell here, and I'm really not sure what to do about it.

The problem, as I see it, is that as of this CL, we're officially using the
HostResolver interface in two different ways.  We are *both* exposing primitives
of resolution (ResolveFromCache, ResolveStaleFromCache) and exposing a "wrap-up"
function Resolve() that exports some behavior that is implementation-dependent. 
The implementation dependent part is fine (it's sorta why abstract base classes
exist), but it's weird that we're doing both in the same interface; I'm used to
the entire interface changing in some reasonable way based on the underlying
implementation, not just one function.

I see this as being basically about the different consumers for HostResolver. 
Within the network stack, we want access to the primitives, to do neat and
interesting things with them (e.g. racing a connection to a stale address with
verifying it with the DNS resolver), but for consumers of the network stack, we
want a simple interface that can be used to "just give me the address associated
with this hostname, already".  Arguably, we shouldn't be using HostResolver for
both of those goals, but I haven't come up with an inheritance model that
wouldn't be over-complex that we could move to. 

Worse, I say "Resolve()" is a wrap-up function, but if I look at the comments in
HostResolver, ResolveStale() is explicitly defined as a function that wraps
Resolve(), and ResolveStaleFromCache() to get a combination of fresh & stale
data.  That comment implicitly documents Resolve() as *not* returning stale
data, so I'm a bit concerned about then wrapping it in a derived class so that
it sometimes *does* return stale data.

So let me make a proposal (but I'm not wedded to this, feel absolutely free to
make a counter-proposal): Create a base class method ResolveFresh() that
explicitly never returns stale data.  Make Resolve() be a pure virtual function
that derived classes have to implement.  Document ResolveStale() as being based
on ResolveFresh() and ResolveStaleFromCache() (though maybe it's name should be
changed so it's not parallel to ResolveFresh()).  This keeps the "One interface
serving two consumers" problem, but it keeps the documentation of the different
interfaces cleaner, at least in my mind. 

WDYT?

[Ryan Sleevi, 2016/05/04 21:14:38]

I'm not going to be able to offer review feedback on this until next  Monday,
but I'm not a big fan of this, although I may not be understanding it.

That said, Randy, use your judgement. I may have misunderstood your arguments,
but alas, I won't have the time today to deal with the meat of them, and I'm OOO
Th/Fri.

[Ryan Sleevi, 2016/05/10 18:59:13]

Right, I totally agree with Randy here.
Right, I totally agree with Randy here.
Right, I totally agree with Randy here.
This is where I got uncomfortable. "One interface serving two consumers" is
still feeling like a nasty code-smell (good ol "Single Responsibility Principle"
in SOLID). Another reason I'm sort of squicky about this is it feels like it
exposes details about resolution strategies on the base class; it really feels
like you should be dealing with a separate object that composes over
HostResolver and does the (ResolveFromCache/Resolve) calls, and I'm not sure
"HostResolver" is the right interface for that.

That said, after re-reading, I think it's better than what's present, but it
still leaves me a bit squicky about the interfaces and behaviours and such.

+  int ResolveFromCache(const RequestInfo& info,
+                       AddressList* addresses,
+                       const BoundNetLog& net_log) override;
+  int ResolveStaleFromCache(const RequestInfo& info,
+                            AddressList* addresses,
+                            HostCache::EntryStaleness* stale_info,
+                            const BoundNetLog& net_log) override;
+  void SetDnsClientEnabled(bool enabled) override;
+  HostCache* GetHostCache() override;
+  std::unique_ptr<base::Value> GetDnsConfigAsValue() const override;
+
+ private:
+  class Request;
+
+  void OnRequestComplete(Request* request,
+                         const CompletionCallback& outer_callback,
+                         int error);
+
+  std::unique_ptr<HostResolver> resolver_;
+  StaleOptions options_;
+  std::set<Request*> pending_requests_;
+};
+
+}  // namespace net
+
+#endif  // NET_DNS_STALE_HOST_RESOLVER_H_