You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by ah...@apache.org on 2026/10/20 13:20:02 UTC
(commons-math) branch master updated (b1bbc2cb1 -> 6df549558)
This is an automated email from the ASF dual-hosted git repository.
aherbert pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/commons-math.git
from b1bbc2cb1 Allow test to fail with OutOfMemoryError
new 208f64fcd Synchronize the remaining mutators of SynchronizedDescriptiveStatistics
new 50ba7180e Simplify javadoc of the synchronized mutators to inheritDoc only
new 6df549558 Synchronize getPercentile of SynchronizedDescriptiveStatistics
The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../SynchronizedDescriptiveStatistics.java | 25 ++++
.../SynchronizedDescriptiveStatisticsTest.java | 127 ++++++++++++++++++++-
2 files changed, 150 insertions(+), 2 deletions(-)
(commons-math) 02/03: Simplify javadoc of the synchronized mutators to inheritDoc only
Posted by ah...@apache.org.
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 50ba7180e77ce6e616c222290760f758f83730b3
Author: Lyin-OTTF <ht...@gmail.com>
AuthorDate: Tue Oct 20 10:29:08 2026 +0800
Simplify javadoc of the synchronized mutators to inheritDoc only
Both removeMostRecentValue and replaceMostRecentValue only need the
inherited documentation; the extra paragraph explaining why the lock is
taken repeats what addValue already conveys and the class-level contract
already states.
---
.../stat/descriptive/SynchronizedDescriptiveStatistics.java | 8 --------
1 file changed, 8 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 4b7fa3391..b580ba0c2 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
@@ -72,10 +72,6 @@ public class SynchronizedDescriptiveStatistics extends DescriptiveStatistics {
/**
* {@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 {
@@ -84,10 +80,6 @@ public class SynchronizedDescriptiveStatistics extends DescriptiveStatistics {
/**
* {@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 {
(commons-math) 01/03: Synchronize the remaining mutators of SynchronizedDescriptiveStatistics
Posted by ah...@apache.org.
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 <30...@qq.com>
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());
+ }
+ }
}
(commons-math) 03/03: Synchronize getPercentile of SynchronizedDescriptiveStatistics
Posted by ah...@apache.org.
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 <ht...@gmail.com>
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 <ht...@gmail.com>
---
.../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);
+ }
}