Cronet: Use stale DNS cache entries experimentally.

components/cronet/stale_host_resolver.h

Index: components/cronet/stale_host_resolver.h
diff --git a/components/cronet/stale_host_resolver.h b/components/cronet/stale_host_resolver.h
new file mode 100644
index 0000000000000000000000000000000000000000..5b939ef8ef12883fa8439adb95407a5804037a46
--- /dev/null
+++ b/components/cronet/stale_host_resolver.h
@@ -0,0 +1,98 @@
+// 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 COMPONENTS_CRONET_STALE_HOST_RESOLVER_H_
+#define COMPONENTS_CRONET_STALE_HOST_RESOLVER_H_
+
+#include <set>
+
+#include "net/dns/host_resolver.h"
+#include "net/dns/host_resolver_impl.h"
+
+namespace cronet {
+
+// A HostResolver that wraps another HostResolver that uses it to make requests

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

nit: We're no longer wrapping an arbitrary HostResolver, so this should probably
refer to HostResolverImpl.

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

Done.

+// 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 net::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<net::HostResolverImpl> inner_resolver,
+                    const StaleOptions& stale_options);
+
+  // Cancels all pending calls to Resolve, and does not call their callbacks.
+  ~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.

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

I'm a bit torn about all the commenting on the methods in this file.  My ideal
image of how implementing a defined interface works is that you document how the
class handles the interface at the top of the class, and any method specific
comments are in the base class file.  On the other hand, that shouldn't hamper
readability.  OTTH, I'm not sure it does.  What would you think about simply
moving the comments that belong there (see notes later) down into the base
class, and just removing all of the other comments and making sure that the
notes describing the class and on the constructor make clear what's special
about how those methods are handled in this class?  I'm happy to go with your
judgement, but I want to raise the issue so you make that judgement.

Separate but related:  You need a note somewhere in here that the following set
of methods are implementing HostResolver.

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

Done.

+  //
+  // |addresses| must remain valid until the request completes (synchronously or
+  // via |callback|) or is canceled (via |CancelRequest| or
+  // |~StaleHostResolver()|).

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

This is actually a comment on the interface, not the implementation, and thus I
think should be in host_resolver.h.

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

nit: Looks like line break in the wrong place?

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

Done.

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

Done.

+  int Resolve(const RequestInfo& info,
+              net::RequestPriority priority,
+              net::AddressList* addresses,
+              const net::CompletionCallback& callback,
+              RequestHandle* out_req,
+              const net::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.

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

This seems like a comment that applies to the interface, not the implementation,
and hence should be in host_resolver.h?

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

Done.

+  void CancelRequest(RequestHandle req) override;
+
+  // The remaining public methods pass through to the inner resolver:
+  int ResolveFromCache(const RequestInfo& info,
+                       net::AddressList* addresses,
+                       const net::BoundNetLog& net_log) override;
+  void SetDnsClientEnabled(bool enabled) override;
+  net::HostCache* GetHostCache() override;
+  std::unique_ptr<base::Value> GetDnsConfigAsValue() const override;
+
+ private:
+  class Request;
+
+  void OnRequestComplete(Request* request,
+                         const net::CompletionCallback& outer_callback,
+                         int error);
+
+  std::unique_ptr<net::HostResolverImpl> resolver_;
+  StaleOptions options_;
+  std::set<Request*> pending_requests_;

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

These are owned by this class, right?  Would std::set<std::unique_ptr<Request>>
work?  I think C++ 11 has a fair amount of support for movable types in the STL
now.

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

It wouldn't be ideal; Requests delete themselves when they finish, so the
creator shouldn't delete them, even (maybe?) at destruction time.

+};
+
+}  // namespace cronet
+
+#endif  // COMPONENTS_CRONET_STALE_HOST_RESOLVER_H_