This is an automated email from the ASF dual-hosted git repository. aherbert pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/commons-math.git
commit 6df549558f13d799698bf3d10e1bca1792d2caee Author: Lyin-OTTF <[email protected]> AuthorDate: Tue Oct 20 15:55:49 2026 +0800 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 <[email protected]> --- .../SynchronizedDescriptiveStatistics.java | 8 ++++ .../SynchronizedDescriptiveStatisticsTest.java | 48 ++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatistics.java b/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatistics.java index b580ba0c2..ea09f9604 100644 --- a/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatistics.java +++ b/commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatistics.java @@ -94,6 +94,14 @@ public class SynchronizedDescriptiveStatistics extends DescriptiveStatistics { return super.apply(stat); } + /** + * {@inheritDoc} + */ + @Override + public synchronized double getPercentile(double p) throws MathIllegalStateException, MathIllegalArgumentException { + return super.getPercentile(p); + } + /** * {@inheritDoc} */ diff --git a/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatisticsTest.java b/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatisticsTest.java index a7ae7db4f..efe7a9a17 100644 --- a/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatisticsTest.java +++ b/commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatisticsTest.java @@ -20,6 +20,7 @@ import java.lang.reflect.Method; 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 final class SynchronizedDescriptiveStatisticsTest extends DescriptiveStat } } + @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 @@ public final class SynchronizedDescriptiveStatisticsTest extends DescriptiveStat 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); + } }
