Alert 222 loop detector improved: do not trigger as long as valid status is received

Patrick observed permature session abort because the Alert 222
loop detector counted 5 Alert 222 messages in less than 20 seconds.

There is a perfectly valid case where this could happen:

- client sends a message to the server with a very compact list of
  deleted items
- server responds with several (> 5) messages that contain the
  Status messages for the deletes, one by one (a bit strange,
  but valid).
- soon the client has no changes left while the server is still
  sending status replies, so the client starts sending Alert
  222.
- After 5 alerts the session was aborted.

The fix is now to reset the 222 alert counter fOutgoingAlert222Count
whenever a essential (i.e. required) status response is received.
So as long as the server sends status, the 222 loop detector
will not trigger.

6 files changed, 26 insertions, 15 deletions

diff --git a/src/sysync/syncagent.cpp b/src/sysync/syncagent.cpp
index 4d3b3b3..73c3dd9 100755
--- a/src/sysync/syncagent.cpp
+++ b/src/sysync/syncagent.cpp
@@ -738,7 +738,7 @@ void TSyncAgent::InternalResetSession(void)
     // (e.g. for date range in func_SetDaysRange())
     fServerHasSINCEBEFORE = true;
     // no outgoing alert 222 sent so far
-    fOutgoingAlertRequests = 0;
+    fOutgoingAlert222Count = 0;
     #endif
   }
   else {
@@ -1283,16 +1283,19 @@ localstatus TSyncAgent::NextMessage(bool &aDone)
        * It is still valid for the server to use ALERT222 to "keep-alive" the
        * connection.
        * Therefore the loop detection criteria is:
-       * 5 adjecent alerts within 20 seconds
+       * - Nothing to send except the 222 Alert (!fNeedToAnswer)
+       * - 5 adjacent 222 alerts within 20 seconds
+       * - no status for an actual sync op command received (fOutgoingAlert222Count will be reset by those)
+       *   because a server sending pending status in small chunks could also trigger the detector otherwise
        */
       if (!fNeedToAnswer) {
         dummyAlert = true;
-        if (fOutgoingAlertRequests++ == 0) {
+        if (fOutgoingAlert222Count++ == 0) {
         	// start of 222 loop detection time
-          fOutgoingAlertStart = getSystemNowAs(TCTX_UTC);
-        } else if (fOutgoingAlertRequests > 5) {
+          fLastOutgoingAlert222 = getSystemNowAs(TCTX_UTC);
+        } else if (fOutgoingAlert222Count > 5) {
           lineartime_t curTime = getSystemNowAs(TCTX_UTC);
-          if (curTime - fOutgoingAlertStart < 20*secondToLinearTimeFactor) {
+          if (curTime - fLastOutgoingAlert222 < 20*secondToLinearTimeFactor) {
             PDEBUGPRINTFX(DBG_ERROR,(
               "Warning: More than 5 consecutive Alert 222 within 20 seconds- "
               "looks like endless loop, abort session"
@@ -1300,7 +1303,7 @@ localstatus TSyncAgent::NextMessage(bool &aDone)
             AbortSession(400, false);
             return getAbortReasonStatus();
           } else {
-            fOutgoingAlertRequests = 0;
+            fOutgoingAlert222Count = 0;
           }
         }
       }
@@ -1318,7 +1321,7 @@ localstatus TSyncAgent::NextMessage(bool &aDone)
   }
   // We send a response with no dummy alert, so reset the alert detector
   if (!dummyAlert) {
-    fOutgoingAlertRequests = 0;
+    fOutgoingAlert222Count = 0;
   }
 
   // send custom end-of session puts
diff --git a/src/sysync/syncagent.h b/src/sysync/syncagent.h
index 16a6f53..8a11522 100755
--- a/src/sysync/syncagent.h
+++ b/src/sysync/syncagent.h
@@ -404,9 +404,12 @@ protected:
   TClientEngineState fClientEngineState;
   #endif // ENGINE_LIBRARY
   // - client side consecutive Alert 222, used to detect endless loop
-  uInt32 fOutgoingAlertRequests;
+  uInt32 fOutgoingAlert222Count;
   // Loop detecting time frame to avoid wrong detection of "keep-alive" message
-  lineartime_t fOutgoingAlertStart;
+  lineartime_t fLastOutgoingAlert222;
+	// - called when essential status was received resets clien side Alert 222 loop detector
+  //   because receiving essential status means real session progress
+  virtual void essentialStatusReceived(void) { fOutgoingAlert222Count = 0; };
 public:
   // - can be cleared to suppress automatic use of DS 1.2 SINCE/BEFORE filters
   //   (e.g. for date range in func_SetDaysRange())
diff --git a/src/sysync/synccommand.cpp b/src/sysync/synccommand.cpp
index 05188da..25f93f2 100755
--- a/src/sysync/synccommand.cpp
+++ b/src/sysync/synccommand.cpp
@@ -1335,8 +1335,8 @@ bool TAlertCommand::issue(
 
 bool TAlertCommand::statusEssential(void)
 {
-	// Alert 222 status is not essential in lenient mode
-  return !(fAlertCode==222 && fSessionP->fLenientMode);
+	// Alert 222 status is not essential
+  return !(fAlertCode==222);
 } // TAlertCommand::statusEssential
 
  
@@ -3340,7 +3340,7 @@ bool TGetCommand::issue(
 
 bool TGetCommand::statusEssential(void)
 {
-	// Alert 222 status is not essential in lenient mode
+	// get status is not essential in lenient mode
   return !(fSessionP->fLenientMode);
 } // TGetCommand::statusEssential
 
diff --git a/src/sysync/synccommand.h b/src/sysync/synccommand.h
index 385b461..578cc15 100755
--- a/src/sysync/synccommand.h
+++ b/src/sysync/synccommand.h
@@ -288,7 +288,7 @@ public:
     uInt32 aInMsgID,     // message ID in which command is being issued
     bool aNoResp=false // issue without wanting response
   ); // returns false, because command can be deleted immediately after issue()
-  virtual bool statusEssential(void);
+  virtual bool statusEssential(void); // depends on alert code if it is essential
   // - add a String Item
   void addItemString(const char *aItemString); // item string to be added
   // - add an Item
@@ -589,7 +589,7 @@ public:
     uInt32 aInMsgID,     // message ID in which command is being issued
     bool aNoResp=true  // status does not want response by default
   ); // returns true if command must be put to the waiting-for-status queue. If false, command can be deleted
-  virtual bool statusEssential(void);
+  virtual bool statusEssential(void); // can be essential or not
   // - handle status received for previously issued command
   //   returns true if done, false if command must be kept in the status queue
   virtual bool handleStatus(TStatusCommand *aStatusCmdP);
diff --git a/src/sysync/syncsession.cpp b/src/sysync/syncsession.cpp
index 904c855..66f9173 100644
--- a/src/sysync/syncsession.cpp
+++ b/src/sysync/syncsession.cpp
@@ -2647,6 +2647,10 @@ Ret_t TSyncSession::handleStatus(TStatusCommand *aStatusCommandP)
               PDEBUGPRINTFX(DBG_SESSION,("Status ignored, command considered done -> deleted"));
             }
             else {
+              // let descendants know when we process a required status
+              if ((*pos)->statusEssential()) {
+                essentialStatusReceived();
+              }
               // normally process status
               if ((*pos)->handleStatus(aStatusCommandP)) {
                 PDEBUGPRINTFX(DBG_SESSION,("Status: processed, removed command '%s' from status wait queue",(*pos)->getName()));
diff --git a/src/sysync/syncsession.h b/src/sysync/syncsession.h
index be08350..1858e70 100755
--- a/src/sysync/syncsession.h
+++ b/src/sysync/syncsession.h
@@ -500,6 +500,7 @@ public:
   // - session continuation and status
   void nextMessageRequest(void);
   bool sessionMustContinue(void);
+  virtual void essentialStatusReceived(void) { /* NOP here */ };
   void delayExecUntilNextRequest(TSmlCommand *aCommand);
   bool delayedSyncEndsPending(void) { return fDelayedExecSyncEnds>0; };
   // - continue interrupted or prevented issue in next package