Limit exponential backoff for broken alternative services.

net/http/http_server_properties_impl.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/http/http_server_properties_impl.h"
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 
 namespace net {
 
 namespace {
 
+// kBrokenAlternativeProtocolDelaySecs << kBrokenDelayMaxShift ~ 2 days.

[Ryan Hamilton, 2016/11/03 16:16:22]

nit: I'm not sure what this comment means? Perhaps a static_assert?

[Bence, 2016/11/07 20:17:24]

Done.

 const uint64_t kBrokenAlternativeProtocolDelaySecs = 300;
+const int kBrokenDelayMaxShift = 9;
 
 }  // namespace
 
 HttpServerPropertiesImpl::HttpServerPropertiesImpl()
     : spdy_servers_map_(SpdyServersMap::NO_AUTO_EVICT),
       alternative_service_map_(AlternativeServiceMap::NO_AUTO_EVICT),
       spdy_settings_map_(SpdySettingsMap::NO_AUTO_EVICT),
       server_network_stats_map_(ServerNetworkStatsMap::NO_AUTO_EVICT),
       quic_server_info_map_(QuicServerInfoMap::NO_AUTO_EVICT),
       max_server_configs_stored_in_properties_(kMaxQuicServersToPersist),
@@ skipping 428 matching lines @@
 }
 
 void HttpServerPropertiesImpl::MarkAlternativeServiceBroken(
     const AlternativeService& alternative_service) {
   // Empty host means use host of origin, callers are supposed to substitute.
   DCHECK(!alternative_service.host.empty());
   if (alternative_service.protocol == UNINITIALIZED_ALTERNATE_PROTOCOL) {
     LOG(DFATAL) << "Trying to mark unknown alternate protocol broken.";
     return;
   }
-  int count = ++recently_broken_alternative_services_[alternative_service];
+  ++recently_broken_alternative_services_[alternative_service];
+  int shift = recently_broken_alternative_services_[alternative_service] - 1;

[Zhongyi Shi, 2016/11/03 15:59:37]

typo here?

Since you're directly retrieving shift, I think what you need here is:
int shift = recently_broken_alternative_services_[alternative_service] ; 
as in the old code, the prefix increment happens before the assignment. Sorry
for not catching this in the first place. 

[Bence, 2016/11/07 20:17:24]

Indeed prefix increment returns new value.  And |count| has the new value in the
old code.  And |shift = count - 1|.  There's many different ways of writing it. 
For example, I can write:

int count = ++r[a];
int shift = count - 1;

Or I can write

int shift = r[a]++;

Or I can write

int shift = r[a];
++r[a];

Or I can write

++r[a];
int shift = r[a] - 1;

These are all equivalent, and I think this is the functionality that want.  Do
you have any preference?  Or I can leave everything as is, and not introduce
|shift|, but then it's not very clear how |count| interacts with
|kBrokenDelayMaxShift| (one is count, the other is shift, and they are off by
one).

+  if (shift > kBrokenDelayMaxShift)
+    shift = kBrokenDelayMaxShift;
   base::TimeDelta delay =
       base::TimeDelta::FromSeconds(kBrokenAlternativeProtocolDelaySecs);
-  base::TimeTicks when = base::TimeTicks::Now() + delay * (1 << (count - 1));
+  base::TimeTicks when = base::TimeTicks::Now() + delay * (1 << shift);
   auto result = broken_alternative_services_.insert(
       std::make_pair(alternative_service, when));
   // Return if alternative service is already in expiration queue.
   if (!result.second) {
     return;
   }
 
   // If this is the only entry in the list, schedule an expiration task.
   // Otherwise it will be rescheduled automatically when the pending task runs.
   if (broken_alternative_services_.size() == 1) {
@@ skipping 310 matching lines @@
   base::TimeDelta delay = when > now ? when - now : base::TimeDelta();
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::Bind(
           &HttpServerPropertiesImpl::ExpireBrokenAlternateProtocolMappings,
           weak_ptr_factory_.GetWeakPtr()),
       delay);
 }
 
 }  // namespace net