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 208f64fcd47797e02d9e75314cdc7b15f2f57090 Author: Lyin-OTTF <[email protected]> AuthorDate: Mon Oct 19 21:38:53 2026 +0800 Synchronize the remaining mutators of SynchronizedDescriptiveStatistics SynchronizedDescriptiveStatistics documents that modification and access methods are atomic operations for a single instance, but it only overrides twelve of the inherited methods. removeMostRecentValue() and replaceMostRecentValue(double) were not among them, so these two mutators updated the storeless statistics without holding the instance monitor - the sibling SynchronizedSummaryStatistics does cover every mutator. A getValues() or getMean() call locks the monitor and copies or computes over the store, so it can now run concurrently with a replace in flight; and a remove can interleave with add so that the discarded value is not the one the caller observed. Both violate the class contract and can produce an inconsistent snapshot rather than an exception. Add synchronized overrides for both methods. The new tests hold the instance monitor and run the mutator on another thread: a synchronized method stays blocked until the monitor is released, whereas the previous implementation returned immediately. A reflection test asserts that every mutator of DescriptiveStatistics is declared synchronized in the wrapper, so a mutator added later cannot silently regress. --- .../SynchronizedDescriptiveStatistics.java | 25 +++++++ .../SynchronizedDescriptiveStatisticsTest.java | 79 +++++++++++++++++++++- 2 files changed, 102 insertions(+), 2 deletions(-) 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 0fc06af95..4b7fa3391 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 @@ -17,6 +17,7 @@ package org.apache.commons.math4.legacy.stat.descriptive; import org.apache.commons.math4.legacy.exception.MathIllegalArgumentException; +import org.apache.commons.math4.legacy.exception.MathIllegalStateException; import org.apache.commons.math4.legacy.exception.NullArgumentException; /** @@ -69,6 +70,30 @@ public class SynchronizedDescriptiveStatistics extends DescriptiveStatistics { super.addValue(v); } + /** + * {@inheritDoc} + * + * <p>This method is a mutator like {@link #addValue(double)}, so it has to take the instance lock as well: + * {@code DescriptiveStatistics} changes the underlying storeless statistics in place, and without the lock a + * concurrent {@code getValues()} could copy the store while it is being updated.</p> + */ + @Override + public synchronized void removeMostRecentValue() throws MathIllegalStateException { + super.removeMostRecentValue(); + } + + /** + * {@inheritDoc} + * + * <p>This method is a mutator like {@link #addValue(double)}, so it has to take the instance lock as well: + * {@code DescriptiveStatistics} changes the underlying storeless statistics in place, and without the lock a + * concurrent {@code getValues()} could copy the store while it is being updated.</p> + */ + @Override + public synchronized double replaceMostRecentValue(double v) throws MathIllegalStateException { + return super.replaceMostRecentValue(v); + } + /** * {@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 512063ab3..a7ae7db4f 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 @@ -16,14 +16,89 @@ */ package org.apache.commons.math4.legacy.stat.descriptive; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import org.junit.Assert; +import org.junit.Test; + /** - * Test cases for the {@link SynchronizedDescriptiveStatisticsTest} class. - * 2007) $ + * Test cases for the {@link SynchronizedDescriptiveStatistics} class. */ public final class SynchronizedDescriptiveStatisticsTest extends DescriptiveStatisticsTest { + /** How long a mutator is observed while the test thread holds the instance monitor. */ + private static final long LOCK_WAIT_MILLIS = 200; + @Override protected DescriptiveStatistics createDescriptiveStatistics() { return new SynchronizedDescriptiveStatistics(); } + + @Test + public void testMutatorsAreSynchronized() throws NoSuchMethodException { + // DescriptiveStatistics updates its storeless statistics in place, so every mutator has to take the + // instance lock on the wrapper; a mutator that is not overridden runs unsynchronized. + final Method[] mutators = { + DescriptiveStatistics.class.getMethod("addValue", double.class), + DescriptiveStatistics.class.getMethod("removeMostRecentValue"), + DescriptiveStatistics.class.getMethod("replaceMostRecentValue", double.class), + DescriptiveStatistics.class.getMethod("clear"), + DescriptiveStatistics.class.getMethod("setWindowSize", int.class), + }; + for (final Method mutator : mutators) { + final Method override = SynchronizedDescriptiveStatistics.class.getDeclaredMethod( + mutator.getName(), mutator.getParameterTypes()); + Assert.assertTrue(mutator.getName() + "() must be synchronized", + Modifier.isSynchronized(override.getModifiers())); + } + } + + @Test + public void testRemoveMostRecentValueWaitsForInstanceLock() throws InterruptedException { + assertWaitsForInstanceLock(false); + } + + @Test + public void testReplaceMostRecentValueWaitsForInstanceLock() throws InterruptedException { + assertWaitsForInstanceLock(true); + } + + /** + * Starts a mutator while the test thread holds the instance monitor and checks that it does not finish + * before the monitor is released. + * + * @param replace {@code true} to call replaceMostRecentValue, {@code false} to call removeMostRecentValue + */ + private void assertWaitsForInstanceLock(final boolean replace) throws InterruptedException { + final SynchronizedDescriptiveStatistics stats = new SynchronizedDescriptiveStatistics(); + stats.addValue(1); + final CountDownLatch started = new CountDownLatch(1); + final CountDownLatch finished = new CountDownLatch(1); + final Thread mutator = new Thread(() -> { + started.countDown(); + if (replace) { + stats.replaceMostRecentValue(2); + } else { + stats.removeMostRecentValue(); + } + finished.countDown(); + }, "mutator"); + synchronized (stats) { + mutator.start(); + Assert.assertTrue("mutator thread did not start", started.await(5, TimeUnit.SECONDS)); + Assert.assertFalse("mutator must wait for the instance lock", + finished.await(LOCK_WAIT_MILLIS, TimeUnit.MILLISECONDS)); + } + Assert.assertTrue("mutator did not complete after the lock was released", + finished.await(5, TimeUnit.SECONDS)); + if (replace) { + Assert.assertEquals(1, stats.getN()); + Assert.assertEquals(2, stats.getElement(0), 0); + } else { + Assert.assertEquals(0, stats.getN()); + } + } }
