From c68a7b81da8a0856546375036799726acf114b16 Mon Sep 17 00:00:00 2001
From: Nischal Sheth <nsheth@juniper.net>
Date: Wed, 26 Aug 2015 17:26:35 -0700
Subject: [PATCH] Improve connection backoff logic in xmpp client

Do not reset the connection attempts counter immediately on reaching
Established state. Do it only after 3 keepalives have been received.

This ensures that the client backs off it's connection attempts even
for cases where the state machine goes to Established, but then goes
down right away.  For example, when the server closes the connection
because some higher level requirement is not satisfied.

Change-Id: Ie6d68c1bef910faf3b7b6adc7a236127ffc9a0ea
Closes-Bug: 1488636
---
 src/xmpp/test/xmpp_client_sm_test.cc | 36 +++++++++++++++++++++++++++-
 src/xmpp/xmpp_state_machine.cc       |  9 +++++--
 src/xmpp/xmpp_state_machine.h        |  9 +++++--
 3 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/src/xmpp/test/xmpp_client_sm_test.cc b/src/xmpp/test/xmpp_client_sm_test.cc
index 27e95b8071a..0a9544d15ba 100644
--- a/src/xmpp/test/xmpp_client_sm_test.cc
+++ b/src/xmpp/test/xmpp_client_sm_test.cc
@@ -135,7 +135,7 @@ class XmppStateMachineTest : public ::testing::Test {
             EXPECT_TRUE(HoldTimerRunning());
             EXPECT_TRUE(sm_->session() != NULL);
             EXPECT_TRUE(connection_->session() != NULL);
-            EXPECT_TRUE(sm_->GetConnectTime() == 0);
+            EXPECT_TRUE(sm_->get_connect_attempts() != 0);
             break;
         default:
             ASSERT_TRUE(false);
@@ -635,6 +635,40 @@ TEST_F(XmppStateMachineTest, Connect_EvTcpConnectFailed_EvTcpConnected) {
     VerifyState(xmsm::OPENSENT);
 }
 
+TEST_F(XmppStateMachineTest, ConnectionBackoff) {
+    sm_->connect_attempts_inc(); // set attempts as we do not
+                                 // want connection timer to expire
+    sm_->connect_attempts_inc(); // set attempts as we do not
+                                 // want connection timer to expire
+    VerifyState(xmsm::IDLE);
+    EvStart();
+    VerifyState(xmsm::ACTIVE);
+
+    for (int idx = 0; idx <= 6; ++idx) {
+        EvConnectTimerExpired();
+        VerifyState(xmsm::CONNECT);
+
+        EvTcpConnected();
+        VerifyState(xmsm::OPENSENT);
+
+        EvXmppOpen();
+        VerifyState(xmsm::ESTABLISHED);
+
+        if (idx == 6)
+            break;
+
+        EvTcpClose();
+        VerifyState(xmsm::ACTIVE);
+    }
+
+    TASK_UTIL_EXPECT_TRUE(sm_->get_connect_attempts() >= 6);
+    EvXmppKeepalive();
+    EvXmppKeepalive();
+    EvXmppKeepalive();
+    TASK_UTIL_EXPECT_EQ(3, sm_->get_keepalive_count());
+    TASK_UTIL_EXPECT_EQ(0, sm_->get_connect_attempts());
+}
+
 }
 
 static void SetUp() {
diff --git a/src/xmpp/xmpp_state_machine.cc b/src/xmpp/xmpp_state_machine.cc
index a0a6c323d64..815ec5fe9de 100644
--- a/src/xmpp/xmpp_state_machine.cc
+++ b/src/xmpp/xmpp_state_machine.cc
@@ -210,6 +210,7 @@ struct Active : public sc::state<Active, XmppStateMachine> {
     Active(my_context ctx) : my_base(ctx) {
         XmppStateMachine *state_machine = &context<XmppStateMachine>();
         SM_LOG(state_machine, "(Xmpp Active State)");
+        state_machine->keepalive_count_clear();
         bool flap = (state_machine->get_state() == ESTABLISHED);
         state_machine->set_state(ACTIVE);
         if (flap) {
@@ -701,7 +702,6 @@ struct XmppStreamEstablished :
     XmppStreamEstablished(my_context ctx) : my_base(ctx) {
         XmppStateMachine *state_machine = &context<XmppStateMachine>();
         SM_LOG(state_machine, "(XMPP Established)");
-        state_machine->connect_attempts_clear();
         XmppConnection *connection = state_machine->connection();
         state_machine->StartHoldTimer();
         state_machine->set_state(ESTABLISHED);
@@ -734,6 +734,10 @@ struct XmppStreamEstablished :
         if (event.session != state_machine->session()) {
             return discard_event();
         }
+        state_machine->keepalive_count_inc();
+        if (state_machine->get_keepalive_count() == 3) {
+            state_machine->connect_attempts_clear();
+        }
         state_machine->StartHoldTimer();
         return discard_event();
     }
@@ -867,6 +871,7 @@ XmppStateMachine::XmppStateMachine(XmppConnection *connection, bool active)
           TaskScheduler::GetInstance()->GetTaskId("xmpp::StateMachine"), 0)),
       hold_time_(GetConfiguredHoldTime()),
       attempts_(0),
+      keepalive_count_(0),
       deleted_(false),
       in_dequeue_(false),
       is_active_(active),
@@ -1165,7 +1170,7 @@ void XmppStateMachine::AssignSession() {
 const int XmppStateMachine::kConnectInterval;
 
 int XmppStateMachine::GetConnectTime() const {
-    int backoff = min(attempts_, 6);
+    int backoff = attempts_ > 6 ? 6 : attempts_;
     return std::min(backoff ? 1 << (backoff - 1) : 0, kConnectInterval);
 }
 
diff --git a/src/xmpp/xmpp_state_machine.h b/src/xmpp/xmpp_state_machine.h
index bac8bdfd0c3..0a133627c0f 100644
--- a/src/xmpp/xmpp_state_machine.h
+++ b/src/xmpp/xmpp_state_machine.h
@@ -112,7 +112,11 @@ class XmppStateMachine :
 
     void connect_attempts_inc() { attempts_++; }
     void connect_attempts_clear() { attempts_ = 0; }
-    int get_connect_attempts() { return attempts_; }
+    int get_connect_attempts() const { return attempts_; }
+
+    void keepalive_count_inc() { keepalive_count_++; }
+    void keepalive_count_clear() { keepalive_count_ = 0; }
+    int get_keepalive_count() const { return keepalive_count_; }
 
     int hold_time() const { return hold_time_; }
     virtual int hold_time_msecs() const { return hold_time_ * 1000; }
@@ -151,7 +155,8 @@ class XmppStateMachine :
     Timer *open_timer_;
     Timer *hold_timer_;
     int hold_time_;
-    int attempts_;
+    uint32_t attempts_;
+    uint32_t keepalive_count_;
     bool deleted_;
     bool in_dequeue_;
     bool is_active_;