DNS: Per-network-type and Finch-variable timeouts

Right now, our DNS timeouts are fixed (6 seconds between calls to thehost resolver, 1 second default retransmission timeout for the internalresolver, and 5 second maximum even with exponential backoff). Thisworks okay on some network types, but could be a lot more patient on
some types (e.g. 2G cell networks).

This CL makes these timeouts configurable via field trials, after which
I willexperiment and find optimal values for each network type, and then
bake in some per-type defaults.

Note that this modifies the behavior of the async resolver in some
cases: if the right field trial is enabled, it will reset accumulated
RTT stats whenever the network *type* changes, not just when the IP
address changes (i.e. the device switches networks entirely).

BUG=595352

Review URL: https://codereview.chromium.org/1778933002

Cr-Commit-Position: refs/heads/master@{#381991}

10 files changed, 148 insertions, 30 deletions

diff --git a/net/dns/dns_config_service.cc b/net/dns/dns_config_service.cc
index cebae36..23f1424 100644
--- a/net/dns/dns_config_service.cc
+++ b/net/dns/dns_config_service.cc
@@ -94,13 +94,13 @@ NameServerClassifier::MergeNameServersTypes(NameServersType a,
 }
 
 // Default values are taken from glibc resolv.h except timeout which is set to
-// |kDnsTimeoutSeconds|.
+// |kDnsDefaultTimeoutMs|.
 DnsConfig::DnsConfig()
     : unhandled_options(false),
       append_to_multi_label_name(true),
       randomize_ports(false),
       ndots(1),
-      timeout(base::TimeDelta::FromSeconds(kDnsTimeoutSeconds)),
+      timeout(base::TimeDelta::FromMilliseconds(kDnsDefaultTimeoutMs)),
       attempts(2),
       rotate(false),
       edns0(false),
diff --git a/net/dns/dns_config_service.h b/net/dns/dns_config_service.h
index 344fdf7..dfe2680 100644
--- a/net/dns/dns_config_service.h
+++ b/net/dns/dns_config_service.h
@@ -30,9 +30,8 @@ namespace net {
 
 class IPAddress;
 
-// Always use 1 second timeout (followed by binary exponential backoff).
-// TODO(szym): Remove code which reads timeout from system.
-const unsigned kDnsTimeoutSeconds = 1;
+// Default to 1 second timeout (before exponential backoff).
+const int64_t kDnsDefaultTimeoutMs = 1000;
 
 // Classifies nameserver address lists for histograms.
 class NET_EXPORT_PRIVATE NameServerClassifier {
diff --git a/net/dns/dns_config_service_posix.cc b/net/dns/dns_config_service_posix.cc
index aaa12b0..b12b019 100644
--- a/net/dns/dns_config_service_posix.cc
+++ b/net/dns/dns_config_service_posix.cc
@@ -150,7 +150,7 @@ ConfigParsePosixResult ReadDnsConfig(DnsConfig* config) {
   }
 #endif  // defined(OS_MACOSX) && !defined(OS_IOS)
   // Override timeout value to match default setting on Windows.
-  config->timeout = base::TimeDelta::FromSeconds(kDnsTimeoutSeconds);
+  config->timeout = base::TimeDelta::FromMilliseconds(kDnsDefaultTimeoutMs);
   return result;
 }
 #else  // defined(OS_ANDROID)
diff --git a/net/dns/dns_session.cc b/net/dns/dns_session.cc
index 5148e0b..6a43eb0 100644
--- a/net/dns/dns_session.cc
+++ b/net/dns/dns_session.cc
@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
+#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sample_vector.h"
 #include "base/rand_util.h"
@@ -20,21 +21,28 @@
 #include "net/base/net_errors.h"
 #include "net/dns/dns_config_service.h"
 #include "net/dns/dns_socket_pool.h"
+#include "net/dns/dns_util.h"
 #include "net/socket/stream_socket.h"
 #include "net/udp/datagram_client_socket.h"
 
 namespace net {
 
 namespace {
-// Never exceed max timeout.
-const unsigned kMaxTimeoutMs = 5000;
+
 // Set min timeout, in case we are talking to a local DNS proxy.
 const unsigned kMinTimeoutMs = 10;
 
+// Default maximum timeout between queries, even with exponential backoff.
+// (Can be overridden by field trial.)
+const unsigned kDefaultMaxTimeoutMs = 5000;
+
+// Maximum RTT that will fit in the RTT histograms.
+const int32_t kRTTMaxMs = 30000;
 // Number of buckets in the histogram of observed RTTs.
-const size_t kRTTBucketCount = 100;
+const size_t kRTTBucketCount = 350;
 // Target percentile in the RTT histogram used for retransmission timeout.
 const unsigned kRTOPercentile = 99;
+
 }  // namespace
 
 // Runtime statistics of DNS server.
@@ -72,7 +80,7 @@ base::LazyInstance<DnsSession::RttBuckets>::Leaky DnsSession::rtt_buckets_ =
     LAZY_INSTANCE_INITIALIZER;
 
 DnsSession::RttBuckets::RttBuckets() : base::BucketRanges(kRTTBucketCount + 1) {
-  base::Histogram::InitializeBucketRanges(1, 5000, this);
+  base::Histogram::InitializeBucketRanges(1, kRTTMaxMs, this);
 }
 
 DnsSession::SocketLease::SocketLease(scoped_refptr<DnsSession> session,
@@ -100,14 +108,40 @@ DnsSession::DnsSession(const DnsConfig& config,
   socket_pool_->Initialize(&config_.nameservers, net_log);
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "AsyncDNS.ServerCount", config_.nameservers.size(), 0, 10, 11);
+  UpdateTimeouts(NetworkChangeNotifier::GetConnectionType());
+  InitializeServerStats();
+  NetworkChangeNotifier::AddConnectionTypeObserver(this);
+}
+
+DnsSession::~DnsSession() {
+  RecordServerStats();
+  NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+}
+
+void DnsSession::UpdateTimeouts(NetworkChangeNotifier::ConnectionType type) {
+  initial_timeout_ = GetTimeDeltaForConnectionTypeFromFieldTrialOrDefault(
+      "AsyncDnsInitialTimeoutMsByConnectionType", config_.timeout, type);
+  max_timeout_ = GetTimeDeltaForConnectionTypeFromFieldTrialOrDefault(
+      "AsyncDnsMaxTimeoutMsByConnectionType",
+      base::TimeDelta::FromMilliseconds(kDefaultMaxTimeoutMs), type);
+}
+
+void DnsSession::InitializeServerStats() {
+  server_stats_.clear();
   for (size_t i = 0; i < config_.nameservers.size(); ++i) {
     server_stats_.push_back(make_scoped_ptr(
-        new ServerStats(config_.timeout, rtt_buckets_.Pointer())));
+        new ServerStats(initial_timeout_, rtt_buckets_.Pointer())));
   }
 }
 
-DnsSession::~DnsSession() {
-  RecordServerStats();
+void DnsSession::OnConnectionTypeChanged(
+    NetworkChangeNotifier::ConnectionType type) {
+  UpdateTimeouts(type);
+  const char* kTrialName = "AsyncDnsFlushServerStatsOnConnectionTypeChange";
+  if (base::FieldTrialList::FindFullName(kTrialName) == "enable") {
+    RecordServerStats();
+    InitializeServerStats();
+  }
 }
 
 uint16_t DnsSession::NextQueryId() const {
@@ -223,9 +257,9 @@ void DnsSession::RecordServerStats() {
 
 
 base::TimeDelta DnsSession::NextTimeout(unsigned server_index, int attempt) {
-  // Respect config timeout if it exceeds |kMaxTimeoutMs|.
-  if (config_.timeout.InMilliseconds() >= kMaxTimeoutMs)
-    return config_.timeout;
+  // Respect initial timeout (from config or field trial) if it exceeds max.
+  if (initial_timeout_ > max_timeout_)
+    return initial_timeout_;
   return NextTimeoutFromHistogram(server_index, attempt);
 }
 
@@ -272,8 +306,7 @@ base::TimeDelta DnsSession::NextTimeoutFromJacobson(unsigned server_index,
   // The timeout doubles every full round.
   unsigned num_backoffs = attempt / config_.nameservers.size();
 
-  return std::min(timeout * (1 << num_backoffs),
-                  base::TimeDelta::FromMilliseconds(kMaxTimeoutMs));
+  return std::min(timeout * (1 << num_backoffs), max_timeout_);
 }
 
 base::TimeDelta DnsSession::NextTimeoutFromHistogram(unsigned server_index,
@@ -303,8 +336,7 @@ base::TimeDelta DnsSession::NextTimeoutFromHistogram(unsigned server_index,
   // The timeout still doubles every full round.
   unsigned num_backoffs = attempt / config_.nameservers.size();
 
-  return std::min(timeout * (1 << num_backoffs),
-                  base::TimeDelta::FromMilliseconds(kMaxTimeoutMs));
+  return std::min(timeout * (1 << num_backoffs), max_timeout_);
 }
 
 }  // namespace net
diff --git a/net/dns/dns_session.h b/net/dns/dns_session.h
index 106493e..b401a64 100644
--- a/net/dns/dns_session.h
+++ b/net/dns/dns_session.h
@@ -16,6 +16,7 @@
 #include "base/metrics/bucket_ranges.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
+#include "net/base/network_change_notifier.h"
 #include "net/base/rand_callback.h"
 #include "net/dns/dns_config_service.h"
 #include "net/dns/dns_socket_pool.h"
@@ -36,7 +37,8 @@ class StreamSocket;
 // Ref-counted so that DnsClient::Request can keep working in absence of
 // DnsClient. A DnsSession must be recreated when DnsConfig changes.
 class NET_EXPORT_PRIVATE DnsSession
-    : NON_EXPORTED_BASE(public base::RefCounted<DnsSession>) {
+    : NON_EXPORTED_BASE(public base::RefCounted<DnsSession>),
+      public NetworkChangeNotifier::ConnectionTypeObserver {
  public:
   typedef base::Callback<int()> RandCallback;
 
@@ -110,7 +112,10 @@ class NET_EXPORT_PRIVATE DnsSession
 
  private:
   friend class base::RefCounted<DnsSession>;
-  ~DnsSession();
+  ~DnsSession() override;
+
+  void UpdateTimeouts(NetworkChangeNotifier::ConnectionType type);
+  void InitializeServerStats();
 
   // Release a socket.
   void FreeSocket(unsigned server_index,
@@ -122,6 +127,10 @@ class NET_EXPORT_PRIVATE DnsSession
   // Compute the timeout using the histogram method.
   base::TimeDelta NextTimeoutFromHistogram(unsigned server_index, int attempt);
 
+  // NetworkChangeNotifier::ConnectionTypeObserver:
+  void OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType type) override;
+
   const DnsConfig config_;
   scoped_ptr<DnsSocketPool> socket_pool_;
   RandCallback rand_callback_;
@@ -130,6 +139,9 @@ class NET_EXPORT_PRIVATE DnsSession
   // Current index into |config_.nameservers| to begin resolution with.
   int server_index_;
 
+  base::TimeDelta initial_timeout_;
+  base::TimeDelta max_timeout_;
+
   struct ServerStats;
 
   // Track runtime statistics of each DNS server.
diff --git a/net/dns/dns_session_unittest.cc b/net/dns/dns_session_unittest.cc
index 1c65c67..e604085 100644
--- a/net/dns/dns_session_unittest.cc
+++ b/net/dns/dns_session_unittest.cc
@@ -219,27 +219,29 @@ TEST_F(DnsSessionTest, AllocateFree) {
   EXPECT_TRUE(NoMoreEvents());
 }
 
-// Expect default calculated timeout to be within 10ms of in DnsConfig.
+// Expect default calculated timeout to be within 10ms of one in DnsConfig.
 TEST_F(DnsSessionTest, HistogramTimeoutNormal) {
   Initialize(2);
-  base::TimeDelta timeoutDelta = session_->NextTimeout(0, 0) - config_.timeout;
-  EXPECT_LT(timeoutDelta.InMilliseconds(), 10);
+  base::TimeDelta delta = session_->NextTimeout(0, 0) - config_.timeout;
+  EXPECT_LE(delta.InMilliseconds(), 10);
 }
 
-// Expect short calculated timeout to be within 10ms of in DnsConfig.
+// Expect short calculated timeout to be within 10ms of one in DnsConfig.
 TEST_F(DnsSessionTest, HistogramTimeoutShort) {
   config_.timeout = base::TimeDelta::FromMilliseconds(15);
   Initialize(2);
-  base::TimeDelta timeoutDelta = session_->NextTimeout(0, 0) - config_.timeout;
-  EXPECT_LT(timeoutDelta.InMilliseconds(), 10);
+  base::TimeDelta delta = session_->NextTimeout(0, 0) - config_.timeout;
+  EXPECT_LE(delta.InMilliseconds(), 10);
 }
 
 // Expect long calculated timeout to be equal to one in DnsConfig.
+// (Default max timeout is 5 seconds, so NextTimeout should return exactly
+// the config timeout.)
 TEST_F(DnsSessionTest, HistogramTimeoutLong) {
   config_.timeout = base::TimeDelta::FromSeconds(15);
   Initialize(2);
   base::TimeDelta timeout = session_->NextTimeout(0, 0);
-  EXPECT_EQ(config_.timeout.InMilliseconds(), timeout.InMilliseconds());
+  EXPECT_EQ(timeout.InMilliseconds(), config_.timeout.InMilliseconds());
 }
 
 }  // namespace
diff --git a/net/dns/dns_util.cc b/net/dns/dns_util.cc
index d2897ff..5f10ad2 100644
--- a/net/dns/dns_util.cc
+++ b/net/dns/dns_util.cc
@@ -9,6 +9,9 @@
 
 #include <cstring>
 
+#include "base/metrics/field_trial.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "build/build_config.h"
 
 #if defined(OS_POSIX)
@@ -152,4 +155,41 @@ bool HaveOnlyLoopbackAddresses() {
 #endif  // defined(various platforms)
 }
 
+#if !defined(OS_NACL)
+namespace {
+
+bool GetTimeDeltaForConnectionTypeFromFieldTrial(
+    const char* field_trial,
+    NetworkChangeNotifier::ConnectionType type,
+    base::TimeDelta* out) {
+  std::string group = base::FieldTrialList::FindFullName(field_trial);
+  if (group.empty())
+    return false;
+  std::vector<base::StringPiece> group_parts = base::SplitStringPiece(
+      group, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  if (type < 0)
+    return false;
+  size_t type_size = static_cast<size_t>(type);
+  if (type_size >= group_parts.size())
+    return false;
+  int64_t ms;
+  if (!base::StringToInt64(group_parts[type_size], &ms))
+    return false;
+  *out = base::TimeDelta::FromMilliseconds(ms);
+  return true;
+}
+
+}  // namespace
+
+base::TimeDelta GetTimeDeltaForConnectionTypeFromFieldTrialOrDefault(
+    const char* field_trial,
+    base::TimeDelta default_delta,
+    NetworkChangeNotifier::ConnectionType type) {
+  base::TimeDelta out;
+  if (!GetTimeDeltaForConnectionTypeFromFieldTrial(field_trial, type, &out))
+    out = default_delta;
+  return out;
+}
+#endif  // !defined(OS_NACL)
+
 }  // namespace net
diff --git a/net/dns/dns_util.h b/net/dns/dns_util.h
index 74f17cc..1acbd66 100644
--- a/net/dns/dns_util.h
+++ b/net/dns/dns_util.h
@@ -8,7 +8,9 @@
 #include <string>
 
 #include "base/strings/string_piece.h"
+#include "base/time/time.h"
 #include "net/base/net_export.h"
+#include "net/base/network_change_notifier.h"
 
 namespace net {
 
@@ -30,6 +32,14 @@ NET_EXPORT_PRIVATE std::string DNSDomainToString(
 // Also returns false if it cannot determine this.
 NET_EXPORT_PRIVATE bool HaveOnlyLoopbackAddresses();
 
+#if !defined(OS_NACL)
+NET_EXPORT_PRIVATE
+base::TimeDelta GetTimeDeltaForConnectionTypeFromFieldTrialOrDefault(
+    const char* field_trial_name,
+    base::TimeDelta default_delta,
+    NetworkChangeNotifier::ConnectionType connection_type);
+#endif  // !defined(OS_NACL)
+
 }  // namespace net
 
 #endif  // NET_DNS_DNS_UTIL_H_
diff --git a/net/dns/host_resolver_impl.cc b/net/dns/host_resolver_impl.cc
index bc1617c..c19d0ac 100644
--- a/net/dns/host_resolver_impl.cc
+++ b/net/dns/host_resolver_impl.cc
@@ -65,6 +65,10 @@ namespace net {
 
 namespace {
 
+// Default delay between calls to the system resolver for the same hostname.
+// (Can be overridden by field trial.)
+const int64_t kDnsDefaultUnresponsiveDelayMs = 6000;
+
 // Limit the size of hostnames that will be resolved to combat issues in
 // some platform's resolvers.
 const size_t kMaxHostLength = 4096;
@@ -1836,7 +1840,8 @@ HostResolverImpl::ProcTaskParams::ProcTaskParams(
     size_t max_retry_attempts)
     : resolver_proc(resolver_proc),
       max_retry_attempts(max_retry_attempts),
-      unresponsive_delay(base::TimeDelta::FromMilliseconds(6000)),
+      unresponsive_delay(
+          base::TimeDelta::FromMilliseconds(kDnsDefaultUnresponsiveDelayMs)),
       retry_factor(2) {
   // Maximum of 4 retry attempts for host resolution.
   static const size_t kDefaultMaxRetryAttempts = 4u;
@@ -1878,12 +1883,15 @@ HostResolverImpl::HostResolverImpl(const Options& options, NetLog* net_log)
   new LoopbackProbeJob(weak_ptr_factory_.GetWeakPtr());
 #endif
   NetworkChangeNotifier::AddIPAddressObserver(this);
+  NetworkChangeNotifier::AddConnectionTypeObserver(this);
   NetworkChangeNotifier::AddDNSObserver(this);
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD) && \
     !defined(OS_ANDROID)
   EnsureDnsReloaderInit();
 #endif
 
+  OnConnectionTypeChanged(NetworkChangeNotifier::GetConnectionType());
+
   {
     DnsConfig dns_config;
     NetworkChangeNotifier::GetDnsConfig(&dns_config);
@@ -1903,6 +1911,7 @@ HostResolverImpl::~HostResolverImpl() {
   STLDeleteValues(&jobs_);
 
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
+  NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
   NetworkChangeNotifier::RemoveDNSObserver(this);
 }
 
@@ -2341,6 +2350,15 @@ void HostResolverImpl::OnIPAddressChanged() {
   // |this| may be deleted inside AbortAllInProgressJobs().
 }
 
+void HostResolverImpl::OnConnectionTypeChanged(
+    NetworkChangeNotifier::ConnectionType type) {
+  proc_params_.unresponsive_delay =
+      GetTimeDeltaForConnectionTypeFromFieldTrialOrDefault(
+          "DnsUnresponsiveDelayMsByConnectionType",
+          base::TimeDelta::FromMilliseconds(kDnsDefaultUnresponsiveDelayMs),
+          type);
+}
+
 void HostResolverImpl::OnInitialDNSConfigRead() {
   UpdateDNSConfig(false);
 }
diff --git a/net/dns/host_resolver_impl.h b/net/dns/host_resolver_impl.h
index fc3835d..5334f7b 100644
--- a/net/dns/host_resolver_impl.h
+++ b/net/dns/host_resolver_impl.h
@@ -62,6 +62,7 @@ class NET_EXPORT HostResolverImpl
     : public HostResolver,
       NON_EXPORTED_BASE(public base::NonThreadSafe),
       public NetworkChangeNotifier::IPAddressObserver,
+      public NetworkChangeNotifier::ConnectionTypeObserver,
       public NetworkChangeNotifier::DNSObserver {
  public:
   // Parameters for ProcTask which resolves hostnames using HostResolveProc.
@@ -240,6 +241,10 @@ class NET_EXPORT HostResolverImpl
   // NetworkChangeNotifier::IPAddressObserver:
   void OnIPAddressChanged() override;
 
+  // NetworkChangeNotifier::ConnectionTypeObserver:
+  void OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType type) override;
+
   // NetworkChangeNotifier::DNSObserver:
   void OnDNSChanged() override;
   void OnInitialDNSConfigRead() override;