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

net/dns/dns_session.cc

Index: net/dns/dns_session.cc
diff --git a/net/dns/dns_session.cc b/net/dns/dns_session.cc
index 5148e0b878b2eba15c598f0673f29b9b699341f2..6b1dbe94f049432457cd4ef77135c4ae7134c312 100644
--- a/net/dns/dns_session.cc
+++ b/net/dns/dns_session.cc
@@ -20,21 +20,40 @@
 #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;
+
+base::TimeDelta GetInitialTimeout(NetworkChangeNotifier::ConnectionType type,

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

Up to you, but from my perspective the contents of this function are simple
enough to hoist into the caller (since it's only called from one place).  Ditto
the next function.

[Deprecated (see juliatuttle), 2016/03/17 16:29:46]

Done.

+                                  base::TimeDelta config_timeout) {
+  return GetTimeDeltaForConnectionTypeFromFieldTrialOrDefault(
+      "AsyncDnsInitialTimeoutMsByConnectionType", config_timeout, type);
+}
+
+base::TimeDelta GetMaxTimeout(NetworkChangeNotifier::ConnectionType type) {
+  return GetTimeDeltaForConnectionTypeFromFieldTrialOrDefault(
+      "AsyncDnsMaxTimeoutMsByConnectionType",
+      base::TimeDelta::FromMilliseconds(kDefaultMaxTimeoutMs), type);
+}
+
 }  // namespace
 
 // Runtime statistics of DNS server.
@@ -72,7 +91,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 +119,37 @@ 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_ = GetInitialTimeout(type, config_.timeout);
+  max_timeout_ = GetMaxTimeout(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) {
+  base::TimeDelta old_initial_timeout = initial_timeout_;
+  UpdateTimeouts(type);
+  if (initial_timeout_ != old_initial_timeout) {
+    RecordServerStats();
+    InitializeServerStats();

[Randy Smith (Not in Mondays), 2016/03/16 20:15:02]

Actually, on my third (:-}) read-through I realize that this may be a
functionality change, if we're sometimes choosing timeouts from the histogram
data.  Naively, it's a *good* change, but I'm both uncomfortable with
functionality changes that aren't called out in the CL description (in case of
non-OWNERS bisecting looking for a breakage), and I'm tempted to suggest that
they really should be in their own CL.  WDYT?

[Deprecated (see juliatuttle), 2016/03/17 16:29:46]

This isn't a functionality change unless a field trial provides differing
timeouts -- if there's no field trial, it won't do anything.

What specifically would you want in a different CL?

[Randy Smith (Not in Mondays), 2016/03/17 21:58:16]

Presuming we resolved this in in-person conversation, but: I'd like to make sure
that the change to reset stats on network changed event is tested separately
from the one where you play with the timeouts, so that any changes in behavior
from the first aren't assumed to be from the second.  You seem to be on track
for doing that, so looks good.

+  }
 }
 
 uint16_t DnsSession::NextQueryId() const {
@@ -223,9 +265,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 +314,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 +344,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