Fixed assertion in HTTPTimeout::lowspeed

mTotalBytes had the meaning of "the sum of all elements in the vector",
but was reset to 0 at the start of a transaction. The meaning is now
changed to "total number of bytes in the vector that belong to the
current transaction".

indra/llmessage/aihttptimeout.cpp

@@ -208,8 +208,9 @@ bool HTTPTimeout::lowspeed(size_t bytes)
   //
   // We do this as follows: we create low_speed_time (in seconds) buckets and fill them with the number
   // of bytes received during that second. We also keep track of the sum of all bytes received between 'now'
-  // and 'now - llmax(starttime, low_speed_time)'. Then if that period reaches at least low_speed_time
+  // and 'llmax(starttime, now - low_speed_time)'. Then if that period reaches at least low_speed_time
-  // seconds, and the transfer rate (sum / low_speed_time) is less than low_speed_limit, we abort.
+  // seconds (when now >= starttime + low_speed_time) and the transfer rate (sum / low_speed_time) is
+  // less than low_speed_limit, we abort.
 
   // When are we?
   S32 second = (sClockCount - mLowSpeedClock) * sClockWidth;
@@ -244,6 +245,7 @@ bool HTTPTimeout::lowspeed(size_t bytes)
   if (s == -1)
   {
 	mBucket = 0;				// It doesn't really matter where we start.
+	mOverwriteSecond = second + low_speed_time;
 	mTotalBytes = bytes;
 	mBuckets[mBucket] = bytes;
 	return false;
@@ -257,11 +259,17 @@ bool HTTPTimeout::lowspeed(size_t bytes)
 	  bucket = 0;
 	if (++s == second)
 	  break;
-    mTotalBytes -= mBuckets[bucket];
+	if (s >= mOverwriteSecond)
+	{
+	  mTotalBytes -= mBuckets[bucket];
+	}
 	mBuckets[bucket] = 0;
   }
   mBucket = bucket;
-  mTotalBytes -= mBuckets[mBucket];
+  if (s >= mOverwriteSecond)
+  {
+	mTotalBytes -= mBuckets[mBucket];
+  }
   mTotalBytes += bytes;
   mBuckets[mBucket] = bytes;
 
@@ -286,29 +294,40 @@ bool HTTPTimeout::lowspeed(size_t bytes)
   }
 
   // Calculate how long the data transfer may stall until we should timeout.
+  //
+  //                Assume 6 buckets:  0 1 2 3 4 5  (low_speed_time == 6)
+  // Seconds since start of transfer:  4 5 6 7 8 9  (mOverwriteSecond == 10)
+  //                  Current second:    ^
+  //                 Data in buckets:  A B w x y z  (mTotalBytes = A + B; w, x, y and z are undefined)
+  //
+  // Obviously, we need to stall at LEAST till second low_speed_time before we can "timeout".
+  // And possibly more if mTotalBytes is already >= mintotalbytes.
+  //
+  // The code below finds 'max_stall_time', so that when from now on the buckets
+  // are filled with 0, then at 'second + max_stall_time' we should time out,
+  // meaning that the resulting mTotalBytes after writing 0 at that second
+  // will be less than mintotalbytes and 'second + max_stall_time' >= low_speed_time.
+  //
   llassert_always(mintotalbytes > 0);
-  S32 max_stall_time = 0;
+  S32 max_stall_time = low_speed_time - second;	// Minimum value.
-  U32 dropped_bytes = 0;
+  // Note that if max_stall_time <= 0 here, then second >= low_speed_time and
-  while(1)
+  // thus mTotalBytes >= mintotalbytes because we didn't timeout already above.
+  if (mTotalBytes >= mintotalbytes)
   {
-	if (++bucket == low_speed_time)				// The next second the next bucket will be emptied.
+	// In this case max_stall_time has a minimum value equal to when we will reach mOverwriteSecond,
-	  bucket = 0;
+	// because that is the first second at which mTotalBytes will decrease.
-	++max_stall_time;
+	max_stall_time = mOverwriteSecond - second - 1;
-	dropped_bytes += mBuckets[bucket];
+	U32 total_bytes = mTotalBytes;
-	// Note how, when max_stall_time == low_speed_time, dropped_bytes has
+	int bucket = -1;		// Must be one less as the start bucket, which corresponds with mOverwriteSecond (and thus with the current max_stall_time).
-	// to be equal to mTotalBytes, the sum of all vector elements.
+	do
-	llassert(max_stall_time < low_speed_time || dropped_bytes == mTotalBytes);
-	// AIFIXME: This is a bug and should really be fixed instead of just be a warning.
-	if (!(max_stall_time < low_speed_time || dropped_bytes == mTotalBytes))
 	{
-	  llwarns << "ASSERT max_stall_time < low_speed_time || dropped_bytes == mTotalBytes failed... aborting. "
+	  ++max_stall_time;					// Next second (mOverwriteSecond upon entry of the loop).
-		"max_stall_time = " << max_stall_time << "; low_speed_time = " << low_speed_time <<
+	  ++bucket;							// The next bucket (0 upon entry of the loop).
-		"; dropped_bytes = " << dropped_bytes << "; mTotalBytes = " << mTotalBytes << llendl;
+	  // Once we reach the end of the vector total_bytes MUST have reached 0 exactly and we should have left this loop.
-	  break;
+	  llassert_always(bucket < low_speed_time);
+	  total_bytes -= mBuckets[bucket];	// Empty this bucket.
 	}
-	// And thus the following will certainly abort.
+	while(total_bytes >= 1);	// Use 1 here instead of mintotalbytes, to test that total_bytes indeed always reaches zero.
-	if (second + max_stall_time >= low_speed_time && mTotalBytes - dropped_bytes < mintotalbytes)
-	  break;
   }
   // If this function isn't called again within max_stall_time seconds, we stalled.
   mStalled = sClockCount + max_stall_time / sClockWidth;

indra/llmessage/aihttptimeout.h

@@ -81,6 +81,7 @@ class HTTPTimeout : public LLRefCount {
 	bool mLowSpeedOn;							// Set while uploading or downloading data.
 	bool mUploadFinished;						// Used to keep track of whether upload_finished was called yet.
 	S32 mLastSecond;							// The time at which lowspeed() was last called, in seconds since mLowSpeedClock.
+	S32 mOverwriteSecond;						// The second at which the first bucket of this transfer will be overwritten.
 	U32 mTotalBytes;							// The sum of all bytes in mBuckets.
 	U64 mLowSpeedClock;							// Clock count at which low speed detection (re)started.
 	U64 mStalled;								// The clock count at which this transaction is considered to be stalling if nothing is transfered anymore.