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

net/dns/dns_session.cc

 // Copyright (c) 2012 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 "net/dns/dns_session.h"
 
 #include <stdint.h>
 #include <limits>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sample_vector.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
 #include "net/base/ip_endpoint.h"
 #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.
 struct DnsSession::ServerStats {
   ServerStats(base::TimeDelta rtt_estimate_param, RttBuckets* buckets)
     : last_failure_count(0), rtt_estimate(rtt_estimate_param) {
     rtt_histogram.reset(new base::SampleVector(buckets));
     // Seed histogram with 2 samples at |rtt_estimate| timeout.
     rtt_histogram->Accumulate(
         static_cast<base::HistogramBase::Sample>(rtt_estimate.InMilliseconds()),
@@ skipping 17 matching lines @@
   scoped_ptr<base::SampleVector> rtt_histogram;
 
   DISALLOW_COPY_AND_ASSIGN(ServerStats);
 };
 
 // static
 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,
                                      unsigned server_index,
                                      scoped_ptr<DatagramClientSocket> socket)
     : session_(session),
       server_index_(server_index),
       socket_(std::move(socket)) {}
 
 DnsSession::SocketLease::~SocketLease() {
   session_->FreeSocket(server_index_, std::move(socket_));
 }
 
 DnsSession::DnsSession(const DnsConfig& config,
                        scoped_ptr<DnsSocketPool> socket_pool,
                        const RandIntCallback& rand_int_callback,
                        NetLog* net_log)
     : config_(config),
       socket_pool_(std::move(socket_pool)),
       rand_callback_(base::Bind(rand_int_callback,
                                 0,
                                 std::numeric_limits<uint16_t>::max())),
       net_log_(net_log),
       server_index_(0) {
   socket_pool_->Initialize(&config_.nameservers, net_log);
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "AsyncDNS.ServerCount", config_.nameservers.size(), 0, 10, 11);
-  for (size_t i = 0; i < config_.nameservers.size(); ++i) {
-    server_stats_.push_back(make_scoped_ptr(
-        new ServerStats(config_.timeout, rtt_buckets_.Pointer())));
-  }
+  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(initial_timeout_, rtt_buckets_.Pointer())));
+  }
+}
+
+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 {
   return static_cast<uint16_t>(rand_callback_.Run());
 }
 
 unsigned DnsSession::NextFirstServerIndex() {
   unsigned index = NextGoodServerIndex(server_index_);
   if (config_.rotate)
     server_index_ = (server_index_ + 1) % config_.nameservers.size();
@@ skipping 95 matching lines @@
       } else {
         UMA_HISTOGRAM_COUNTS("AsyncDNS.ServerFailuresAfterSuccess",
                              server_stats_[index]->last_failure_count);
       }
     }
   }
 }
 
 
 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);
 }
 
 // Allocate a socket, already connected to the server address.
 scoped_ptr<DnsSession::SocketLease> DnsSession::AllocateSocket(
     unsigned server_index, const NetLog::Source& source) {
   scoped_ptr<DatagramClientSocket> socket;
 
   socket = socket_pool_->AllocateSocket(server_index);
   if (!socket.get())
@@ skipping 26 matching lines @@
   DCHECK_LT(server_index, server_stats_.size());
 
   base::TimeDelta timeout = server_stats_[server_index]->rtt_estimate +
                             4 * server_stats_[server_index]->rtt_deviation;
 
   timeout = std::max(timeout, base::TimeDelta::FromMilliseconds(kMinTimeoutMs));
 
   // 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,
                                                      int attempt) {
   DCHECK_LT(server_index, server_stats_.size());
 
   static_assert(std::numeric_limits<base::HistogramBase::Count>::is_signed,
                 "histogram base count assumed to be signed");
 
   // Use fixed percentile of observed samples.
   const base::SampleVector& samples =
       *server_stats_[server_index]->rtt_histogram;
 
   base::HistogramBase::Count total = samples.TotalCount();
   base::HistogramBase::Count remaining_count = kRTOPercentile * total / 100;
   size_t index = 0;
   while (remaining_count > 0 && index < rtt_buckets_.Get().size()) {
     remaining_count -= samples.GetCountAtIndex(index);
     ++index;
   }
 
   base::TimeDelta timeout =
       base::TimeDelta::FromMilliseconds(rtt_buckets_.Get().range(index));
 
   timeout = std::max(timeout, base::TimeDelta::FromMilliseconds(kMinTimeoutMs));
 
   // 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