Backoff: Respect maximum delay in face of float overflow

net/base/backoff_entry.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/base/backoff_entry.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 
@@ skipping 116 matching lines @@
     // Never reduce previously set release horizon, e.g. due to Retry-After
     // header.
     return std::max(ImplGetTimeNow(), exponential_backoff_release_time_);
   }
 
   // The delay is calculated with this formula:
   // delay = initial_backoff * multiply_factor^(
   //     effective_failure_count - 1) * Uniform(1 - jitter_factor, 1]
   double delay = policy_->initial_delay_ms;
   delay *= pow(policy_->multiply_factor, effective_failure_count - 1);
+  if (isinf(delay)) {
+    delay = std::max(policy_->maximum_backoff_ms,

[cbentzel, 2014/06/14 15:16:07]

maximum_backoff_ms is not always set - so it seems like a bad idea to go to
initial_delay_ms here.

Instead, I think this could be written like

-----

delay *= pow(policy...);
const int64 kMaxInt64 = std::numeric_limits<int64>::max();
int64 delay_int = 0;
if (isinf(delay)) {
  delay_int = kMaxInt64;
} else {
  delay -= base::RandDouble() * policy_->jitter_factory * delay;
  delay_int = (delay > kMaxInt64) ? kMaxInt64 : static_cast<int64>(delay + 0.5);
}
if (policy_->maximum_backoff_ms >= 0)
  delay_int = std::min(delay_int, policy_->maximum_backoff_ms);

------

In the overflow case this means that we will either be the maximum_backoff_ms or
kMaxInt64. Other case stays the same.

-------

However - I am worried that we may get integral overflow issues at some point if
exponential_backoff_release_time_ is kMaxInt64. Inside this class it does not
look like that will be a problem, but we do expose it (via GetReleaseTime) out
to callers and they may run into this issue.

[cbentzel, 2014/06/14 15:30:47]

Actually, we'd reach integral overflow with this approach below when doing
ImplGetTimeNow() + delay_int

[Ryan Sleevi, 2014/06/16 17:20:47]

^ base/basictypes.h has kint64max / kint64min
^ Pretty sure it could still overflow here

[Nicolas Zea, 2014/06/16 20:27:25]

Switched to doing all overflow checking via CheckedNumeric and in microseconds
(to ensure the TimeTicks conversion doesn't overflow).

+                     static_cast<int64>(policy_->initial_delay_ms));
+  }
   delay -= base::RandDouble() * policy_->jitter_factor * delay;
 
   const int64 kMaxInt64 = std::numeric_limits<int64>::max();
   int64 delay_int = (delay > kMaxInt64) ?
       kMaxInt64 : static_cast<int64>(delay + 0.5);
 
   // Ensure that we do not exceed maximum delay.
   if (policy_->maximum_backoff_ms >= 0)
     delay_int = std::min(delay_int, policy_->maximum_backoff_ms);
 
   // Never reduce previously set release horizon, e.g. due to Retry-After
   // header.
   return std::max(
       ImplGetTimeNow() + base::TimeDelta::FromMilliseconds(delay_int),
       exponential_backoff_release_time_);
 }
 
 }  // namespace net