Synchronize getPercentile of SynchronizedDescriptiveStatistics

DescriptiveStatistics.getPercentile sets the quantile on the shared
percentile implementation and then calls apply, which is synchronized
in SynchronizedDescriptiveStatistics. The setQuantile step ran outside
the instance lock, so a concurrent getPercentile call could swap the
quantile in between the two steps and a caller could end up computing
a percentile for a quantile it never requested.

Override getPercentile as synchronized so the quantile update and the
apply are atomic with respect to the other synchronized methods on the
same instance. Add a reflection test guarding the synchronized modifier
and a concurrency test that holds the instance monitor while another
thread requests a different quantile, asserting the requested quantile
is the one applied.

Signed-off-by: Lyin-OTTF <htao08203@gmail.com>

Author: Lyin-OTTF

commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatistics.java

@@ -94,6 +94,14 @@ public synchronized double apply(UnivariateStatistic stat) {
         return super.apply(stat);
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public synchronized double getPercentile(double p) throws MathIllegalStateException, MathIllegalArgumentException {
+        return super.getPercentile(p);
+    }
+
     /**
      * {@inheritDoc}
      */

commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatisticsTest.java

@@ -20,6 +20,7 @@
 import java.lang.reflect.Modifier;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.Assert;
 import org.junit.Test;
@@ -56,6 +57,17 @@ public void testMutatorsAreSynchronized() throws NoSuchMethodException {
         }
     }
 
+    @Test
+    public void testGetPercentileIsSynchronized() throws NoSuchMethodException {
+        // DescriptiveStatistics.getPercentile sets the shared percentile quantile before calling the
+        // synchronized apply, so the whole method (setQuantile + apply) has to take the instance lock;
+        // otherwise a concurrent call can swap the quantile in between the two steps.
+        final Method getPercentile = SynchronizedDescriptiveStatistics.class.getDeclaredMethod(
+                "getPercentile", double.class);
+        Assert.assertTrue("getPercentile(double) must be synchronized",
+                Modifier.isSynchronized(getPercentile.getModifiers()));
+    }
+
     @Test
     public void testRemoveMostRecentValueWaitsForInstanceLock() throws InterruptedException {
         assertWaitsForInstanceLock(false);
@@ -101,4 +113,40 @@ private void assertWaitsForInstanceLock(final boolean replace) throws Interrupte
             Assert.assertEquals(0, stats.getN());
         }
     }
+
+    @Test
+    public void testGetPercentileAppliesRequestedQuantileAtomically() throws InterruptedException {
+        final SynchronizedDescriptiveStatistics stats = new SynchronizedDescriptiveStatistics();
+        final DescriptiveStatistics reference = new DescriptiveStatistics();
+        for (int i = 1; i <= 100; i++) {
+            stats.addValue(i);
+            reference.addValue(i);
+        }
+        final double expectedQuartile = reference.getPercentile(25);
+        final double expectedTop = reference.getPercentile(90);
+        Assert.assertTrue("test setup: the two quantiles must differ", expectedQuartile != expectedTop);
+
+        final AtomicReference<Double> other = new AtomicReference<>();
+        final Thread percentileCaller = new Thread(() -> other.set(stats.getPercentile(25)), "percentile-caller");
+        synchronized (stats) {
+            percentileCaller.start();
+            // Hold the instance monitor and wait until the other thread blocks trying to acquire it.
+            // If getPercentile were not synchronized, the other thread would already have set the
+            // quantile to 25 outside the lock and be parked inside apply(); with the synchronized
+            // override it parks at the method entry, so our own call cannot disturb its quantile.
+            final long deadline = System.currentTimeMillis() + 5000;
+            while (percentileCaller.getState() != Thread.State.BLOCKED
+                    && System.currentTimeMillis() < deadline) {
+                Thread.sleep(1);
+            }
+            Assert.assertEquals("other thread should be blocked on the instance monitor",
+                    Thread.State.BLOCKED, percentileCaller.getState());
+            Assert.assertEquals("our own call must still see the requested quantile",
+                    expectedTop, stats.getPercentile(90), 0);
+        }
+        percentileCaller.join(5000);
+        Assert.assertFalse("other thread did not complete", percentileCaller.isAlive());
+        Assert.assertEquals("getPercentile must apply the requested quantile atomically",
+                expectedQuartile, other.get(), 0);
+    }
 }