Implement throttling logic for fetching affiliation information.

components/password_manager/core/browser/affiliation_fetch_throttler.cc

+// Copyright 2015 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 "components/password_manager/core/browser/affiliation_fetch_throttler.h"
+
+#include <stdint.h>
+
+#include "base/logging.h"
+#include "base/rand_util.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/time/tick_clock.h"
+#include "base/time/time.h"
+#include "components/password_manager/core/browser/affiliation_fetch_throttler_delegate.h"
+#include "net/base/backoff_entry.h"
+
+namespace password_manager {
+
+namespace {
+
+const net::BackoffEntry::Policy kAffiliationBackoffParameters = {
+    // Number of initial errors (in sequence) to ignore before going into
+    // exponential backoff.
+    0,
+
+    // Initial delay (in ms) once backoff starts.
+    10 * 1000,  // 10 seconds
+
+    // Factor by which the delay will be multiplied on each subsequent failure.
+    4,
+
+    // Fuzzing percentage: 50% will spread delays randomly between 50%--100% of
+    // the nominal time.
+    .5,  // 50%
+
+    // Maximum delay (in ms) during exponential backoff.
+    6 * 3600 * 1000,  // 6 hours
+
+    // Time to keep an entry from being discarded even when it has no
+    // significant state, -1 to never discard. (Not applicable.)
+    -1,
+
+    // False means that initial_delay_ms is the first delay once we start
+    // exponential backoff, i.e., there is no delay after subsequent successful
+    // requests.
+    false,
+};
+
+// Grace period before the first request is sent after network connectivity is
+// restored. The fuzzing factor from above applies.
+const int64_t kGracePeriodAfterNetworkRestoredInMs = 10 * 1000;  // 10 seconds
+
+// Implementation of net::BackoffEntry that allows mocking its tick source.
+class BackoffEntryImpl : public net::BackoffEntry {
+ public:
+  explicit BackoffEntryImpl(base::TickClock* tick_clock)
+      : BackoffEntry(&kAffiliationBackoffParameters), tick_clock_(tick_clock) {}
+  ~BackoffEntryImpl() override {}
+
+ private:
+  // net::BackoffEntry:
+  base::TimeTicks ImplGetTimeNow() const override {
+    return tick_clock_->NowTicks();
+  }
+
+  // Must outlive this instance.
+  base::TickClock* tick_clock_;
+
+  DISALLOW_COPY_AND_ASSIGN(BackoffEntryImpl);
+};
+
+}  // namespace
+
+AffiliationFetchThrottler::AffiliationFetchThrottler(
+    AffiliationFetchThrottlerDelegate* delegate,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    scoped_ptr<base::TickClock> tick_clock)
+    : delegate_(delegate),
+      task_runner_(task_runner),
+      state_(IDLE),
+      has_network_connectivity_(false),
+      is_fetch_scheduled_(false),
+      tick_clock_(tick_clock.Pass()),
+      exponential_backoff_(new BackoffEntryImpl(tick_clock_.get())),
+      weak_ptr_factory_(this) {
+  DCHECK(delegate);
+  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
+  has_network_connectivity_ = !net::NetworkChangeNotifier::IsOffline();

[mmenke, 2015/01/16 21:28:34]

Think it's worth a comment why this shouldn't be in the initializer list (Has to
be done after being added as an observer...)

[engedy, 2015/01/19 18:13:11]

Done.

+}
+
+AffiliationFetchThrottler::~AffiliationFetchThrottler() {
+  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+}
+
+void AffiliationFetchThrottler::SignalNetworkRequestNeeded() {
+  if (state_ != IDLE)

[mmenke, 2015/01/16 21:28:35]

Should we DCHECK on this?  I assume one class will own |this| and be solely
responsible for talking to it..Seems kinda weird to call this multiple times,
though I have no idea about what we're actually getting.

Either way, if it's allowed or not, should have some docs about it.

[engedy, 2015/01/19 18:13:10]

Being able to call this multiple times allows the caller to not keep track of
whether or not it has already signaled that a request is needed. Added comments
to clarify this.

+    return;
+
+  state_ = FETCH_NEEDED;

[mmenke, 2015/01/16 21:28:35]

Does the delegate have to be able to handle a call to OnCanSendNetworkRequest
while a fetch is in flight?  If not, you're going to need a new state where a
fetch is needed *and* a fetch is in flight - two bools would make more sense, in
that case.

[mmenke, 2015/01/16 21:49:54]

Hrm...Was thinking there was a case where that could happen, but looks like I
was mistaken.

[engedy, 2015/01/19 18:13:11]

Yes, I believe this can never happen. I have added a class comment to clarify
that the class supports one request at a time.

+  if (has_network_connectivity_)
+    EnsureCallbackIsScheduled();
+}
+
+void AffiliationFetchThrottler::InformOfNetworkRequestComplete(bool success) {
+  DCHECK_EQ(state_, FETCH_IN_FLIGHT);

[mmenke, 2015/01/16 21:28:34]

Suggest making expected value the first argument (It's ASSERT_EQ/EXPECT_EQ where
it's required, but I think it's best to be consistent).  Goes for the others in
this file as well.

[engedy, 2015/01/19 18:13:11]

I'd prefer to keep it this way. As opposed to EXPECT_EQ(), DCHECK_EQ() makes no
distinction between its arguments, and having them listed in this order will
result in the more readable output:

  Check failed: state_ == FETCH_NEEDED (1 vs. 2)

+  state_ = IDLE;
+  exponential_backoff_->InformOfRequest(success);

[mmenke, 2015/01/16 21:31:28]

It's up to the consumer to tell us if/when they want to try again, as opposed to
automatically triggering retries, with backoff?  Think that's also worth
mentioning in the header.

[engedy, 2015/01/19 18:13:11]

Done. PTAL at the class comment.

+}
+
+void AffiliationFetchThrottler::EnsureCallbackIsScheduled() {
+  DCHECK_EQ(state_, FETCH_NEEDED);

[mmenke, 2015/01/16 21:49:54]

DCHECK(has_network_connectivity_)?

[engedy, 2015/01/19 18:13:11]

Done.

+  if (is_fetch_scheduled_)
+    return;
+
+  is_fetch_scheduled_ = true;

[mmenke, 2015/01/16 21:28:34]

Suggest using a OneShotTimer instead, it keeps track of this, and makes life
simpler (And less regression prone, in my opinion).  Can also easily stop it or
restart it when connectivity changes, which I think makes life simpler.

[engedy, 2015/01/19 18:13:11]

I wanted to use that, but it does not allow time to be mocked out, which is
needed for the unit tests.

[mmenke, 2015/01/21 17:18:52]

Ah, I wasn't aware we had a task runner class with mockable time.

+  task_runner_->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&AffiliationFetchThrottler::OnBackoffDelayExpiredCallback,
+                 weak_ptr_factory_.GetWeakPtr()),
+      exponential_backoff_->GetTimeUntilRelease());

[mmenke, 2015/01/16 21:49:54]

Also, do we need async behavior here?  Seems like we have a lot more checks than
needed here.  If we could just have:

if (!has_network_connectivity_ || is_fetch_scheduled_)
  return;

if (exponential_backoff_->ShouldRejectRequest())
  // Post task
else
  // run task, update state

Could get rid of OnBackoffDelayExpiredCallback, remove some logic from
AffiliationFetchThrottler::OnConnectionTypeChanged, and remove an if from
SignalNetworkRequestNeeded.

[engedy, 2015/01/19 18:13:11]

While we could certainly do that, do you think putting all that logic into a
single method would be more readable?

I usually get suspicious when I cannot come up with a name for a method, and I
cannot seem to.

[mmenke, 2015/01/21 17:18:52]

ScheduleCallbackIfNeeded?  (Could even have the state_ == FETCH_NEEDED in
there...  Or could move some of the checks, and keep the current always async
behavior).  Anyhow, up to you.

I grow suspicious if I see very similar logic repeated in a file a couple times,
though, admittedly, it isn't too bad.

[engedy, 2015/01/23 22:09:43]

I would like to keep always async behavior. On that precondition I do not see
any substantial simplification opportunities (the ones I see would hinder
readability).

+}
+
+void AffiliationFetchThrottler::OnBackoffDelayExpiredCallback() {
+  DCHECK_EQ(state_, FETCH_NEEDED);
+  DCHECK(is_fetch_scheduled_);
+  is_fetch_scheduled_ = false;
+
+  // Do nothing if network connectivity was lost while this callback was in the
+  // task queue. The callback will be posted in the OnConnectionTypeChanged
+  // handler once again.
+  if (!has_network_connectivity_)
+    return;
+
+  // The release time might have been increased if network connectivity was lost
+  // and restored while this callback was in the task queue. If so, reschedule.
+  if (exponential_backoff_->ShouldRejectRequest())
+    EnsureCallbackIsScheduled();
+  else
+    state_ = delegate_->OnCanSendNetworkRequest() ? FETCH_IN_FLIGHT : IDLE;
+}
+
+void AffiliationFetchThrottler::OnConnectionTypeChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  bool old_has_network_connectivity = has_network_connectivity_;
+  has_network_connectivity_ =
+      (type != net::NetworkChangeNotifier::CONNECTION_NONE);
+
+  // Only react when network connectivity has been reestablished.
+  if (!has_network_connectivity_ || old_has_network_connectivity)
+    return;
+
+  double grace_ms =
+      kGracePeriodAfterNetworkRestoredInMs *
+      (1 - base::RandDouble() * kAffiliationBackoffParameters.jitter_factor);
+  base::TimeTicks release_time = std::max(
+      exponential_backoff_->GetReleaseTime(),
+      tick_clock_->NowTicks() + base::TimeDelta::FromMillisecondsD(grace_ms));
+  // Note that SetCustomReleaseTime() takes the maximum of the current release
+  // time and the supplied |release_time|.
+  exponential_backoff_->SetCustomReleaseTime(release_time);
+
+  if (state_ == FETCH_NEEDED)
+    EnsureCallbackIsScheduled();
+}
+
+}  // namespace password_manager