Merge pull request #385 from brharrington/issue-384

fix overflow issues with StatsBuffer

servo-core/src/main/java/com/netflix/servo/stats/StatsBuffer.java

@@ -25,9 +25,9 @@
  * This implementation is not thread safe.
  */
 public class StatsBuffer {
-  private int count;
+  private int pos;
+  private int curSize;
   private double mean;
-  private double sumSquares;
   private double variance;
   private double stddev;
   private long min;
@@ -76,14 +76,14 @@ private static boolean validPercentiles(double[] percentiles) {
    */
   public void reset() {
     statsComputed.set(false);
-    count = 0;
+    pos = 0;
+    curSize = 0;
     total = 0L;
     mean = 0.0;
     variance = 0.0;
     stddev = 0.0;
     min = 0L;
     max = 0L;
-    sumSquares = 0.0;
     for (int i = 0; i < percentileValues.length; ++i) {
       percentileValues[i] = 0.0;
     }
@@ -93,9 +93,10 @@ public void reset() {
    * Record a new value for this buffer.
    */
   public void record(long n) {
-    values[count++ % size] = n;
-    total += n;
-    sumSquares += n * n;
+    values[Integer.remainderUnsigned(pos++, size)] = n;
+    if (curSize < size) {
+      ++curSize;
+    }
   }
 
   /**
@@ -106,17 +107,26 @@ public void computeStats() {
       return;
     }
 
-    if (count == 0) {
+    if (curSize == 0) {
       return;
     }
 
-    int curSize = Math.min(count, size);
     Arrays.sort(values, 0, curSize); // to compute percentileValues
     min = values[0];
     max = values[curSize - 1];
-    mean = (double) total / count;
+
+    // TODO: This should probably be changed to use Welford's method to
+    // compute the variance.
+    total = 0L;
+    double sumSquares = 0.0;
+    for (int i = 0; i < curSize; ++i) {
+      total += values[i];
+      sumSquares += values[i] * values[i];
+    }
+    mean = (double) total / curSize;
     variance = (sumSquares / curSize) - (mean * mean);
     stddev = Math.sqrt(variance);
+
     computePercentiles(curSize);
   }
 
@@ -155,10 +165,10 @@ private double calcPercentile(int curSize, double percent) {
   }
 
   /**
-   * Get the number of entries recorded.
+   * Get the number of entries recorded up to the size of the buffer.
    */
   public int getCount() {
-    return count;
+    return curSize;
   }
 
   /**

servo-core/src/test/java/com/netflix/servo/stats/StatsBufferTest.java

@@ -17,6 +17,8 @@
 
 import org.testng.annotations.Test;
 
+import java.lang.reflect.Field;
+
 import static org.testng.Assert.assertEquals;
 
 public class StatsBufferTest {
@@ -154,10 +156,10 @@ public void testMinWrap() {
   @Test
   public void testCountWrap() {
     StatsBuffer buffer = getWithWrap();
-    assertEquals(buffer.getCount(), SIZE * 2);
+    assertEquals(buffer.getCount(), SIZE);
   }
 
-  static final long EXPECTED_TOTAL_WRAP = SIZE * 2 * (SIZE * 2 + 1) / 2;
+  static final long EXPECTED_TOTAL_WRAP = SIZE * (SIZE + 1) / 2;
 
   @Test
   public void testTotalWrap() {
@@ -168,10 +170,10 @@ public void testTotalWrap() {
   @Test
   public void testMeanWrap() {
     StatsBuffer buffer = getWithWrap();
-    assertEquals(buffer.getMean(), (double) EXPECTED_TOTAL_WRAP / (SIZE * 2));
+    assertEquals(buffer.getMean(), (double) EXPECTED_TOTAL_WRAP / SIZE);
   }
 
-  static final double EXPECTED_VARIANCE_WRAP = 1667666.75;
+  static final double EXPECTED_VARIANCE_WRAP = 83333.25;
 
   @Test
   public void testVarianceWrap() {
@@ -214,5 +216,31 @@ public void testPercentiles995Wrap() {
     assertEquals(percentiles[3], 996.0);
   }
 
+  // Used to access private count field via reflection so we can quickly simulate
+  // a count that will cause an integer overflow.
+  private void setCount(StatsBuffer buffer, int v) throws Exception {
+    Class<?> cls = buffer.getClass();
+    Field field = cls.getDeclaredField("pos");
+    field.setAccessible(true);
+    field.set(buffer, v);
+  }
+
+  // Before fix this would throw an ArrayIndexOutOfBoundException
+  @Test
+  public void testCountOverflow() throws Exception {
+    StatsBuffer buffer = new StatsBuffer(SIZE, PERCENTILES);
+    setCount(buffer, Integer.MAX_VALUE);
+    buffer.record(1);
+    buffer.record(2);
+  }
 
+  // java.lang.IllegalArgumentException: fromIndex(0) > toIndex(-2147483647)
+  @Test
+  public void testComputeStatsWithOverflow() throws Exception {
+    StatsBuffer buffer = new StatsBuffer(SIZE, PERCENTILES);
+    setCount(buffer, Integer.MAX_VALUE);
+    buffer.record(1);
+    buffer.record(2);
+    buffer.computeStats();
+  }
 }