You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Lyin-OTTF (via GitHub)" <gi...@apache.org> on 2026/10/19 13:39:01 UTC
[PR] Synchronize the remaining mutators of SynchronizedDescriptiveStatistics [commons-math]
Lyin-OTTF opened a new pull request, #333:
URL: https://github.com/apache/commons-math/pull/333
`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)` are missing, so these two mutators update the storeless statistics **without holding the instance monitor**. The sibling `SynchronizedSummaryStatistics` covers every mutator, so this looks like an oversight rather than a deliberate exception.
Why it matters:
- `getValues()`, `getMean()`, `getPercentile()` and the other accessors lock the monitor (directly or through the overridden `apply()`), so they can now copy or compute over the store while a replace is in flight - the returned snapshot can mix pre- and post-replace values.
- `removeMostRecentValue()` can interleave with `addValue()`, discarding a different element than the caller expects, and with windowed statistics the window bookkeeping can end up inconsistent.
- These are the only two `DescriptiveStatistics` methods that mutate state and do not take the lock; everything else (`addValue`, `clear`, `setWindowSize`, the `setXxxImpl` setters) already does.
Changes:
- `SynchronizedDescriptiveStatistics` overrides both mutators with `synchronized`, mirroring the existing overrides.
- `SynchronizedDescriptiveStatisticsTest` gains three cases:
- `testMutatorsAreSynchronized` reflects over the mutators of `DescriptiveStatistics` and asserts each is declared `synchronized` in the wrapper, so a mutator added later cannot silently regress;
- `testRemoveMostRecentValueWaitsForInstanceLock` / `testReplaceMostRecentValueWaitsForInstanceLock` hold the instance monitor and run the mutator on another thread, then assert it does not complete until the monitor is released. On the previous implementation the calls return immediately and the assertions fail.
- Also fixes the class Javadoc, which linked to the test class instead of the class under test.
Verification (Windows, JDK 21): `mvn -pl commons-math-legacy -am test -Dtest=SynchronizedDescriptiveStatisticsTest` passes (20/20, including the 17 inherited cases); reverting only `SynchronizedDescriptiveStatistics.java` fails exactly the three new cases. `mvn -pl commons-math-legacy checkstyle:check` passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Synchronize the remaining mutators of SynchronizedDescriptiveStatistics [commons-math]
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on code in PR #333:
URL: https://github.com/apache/commons-math/pull/333#discussion_r4316071488
##########
commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatistics.java:
##########
@@ -69,6 +70,30 @@ public synchronized void addValue(double v) {
super.addValue(v);
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method is a mutator like {@link #addValue(double)}, so it has to take the instance lock as well:
Review Comment:
This comment is not required. Using `{@inheritDoc}` is fine.
##########
commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/descriptive/SynchronizedDescriptiveStatistics.java:
##########
@@ -69,6 +70,30 @@ public synchronized void addValue(double v) {
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:
Review Comment:
This comment is not required. Using `{@inheritDoc}` is fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Synchronize the remaining mutators of SynchronizedDescriptiveStatistics [commons-math]
Posted by "Lyin-OTTF (via GitHub)" <gi...@apache.org>.
Lyin-OTTF commented on PR #333:
URL: https://github.com/apache/commons-math/pull/333#issuecomment-6286259348
@aherbert Done in df21ddf.
Synchronized getPercentile in SynchronizedDescriptiveStatistics. DescriptiveStatistics.getPercentile sets the quantile on the shared percentile impl and then calls apply, which is synchronized on the wrapper; without synchronizing the whole method another thread could swap the quantile between the setQuantile and the apply, so a caller could compute a percentile for a quantile it never requested. The override makes both steps atomic against the other synchronized methods.
Added two tests: one asserts getPercentile is declared synchronized (same style as the existing mutator check), the other holds the instance monitor while a second thread requests the 25th percentile and the main thread requests the 90th, then asserts the second thread still observes the 25th. SynchronizedDescriptiveStatisticsTest is now 22 tests, all green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Synchronize the remaining mutators of SynchronizedDescriptiveStatistics [commons-math]
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert commented on PR #333:
URL: https://github.com/apache/commons-math/pull/333#issuecomment-6285288248
This looks good thank you.
I note that in the `DescriptiveStatistics` class the method `getPercentile` updates the `PercentileImpl` before calling the `apply` method which is synchronized in `SynchronizedDescriptiveStatistics`. So `SynchronizedDescriptiveStatistics` can change the quantile being used in the percentile computation from another thread, then wait for the computation to finish before starting another one.
Please also synchronize the `gerPercentile` method. Adding a test for this would be nice to have.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Synchronize the remaining mutators of SynchronizedDescriptiveStatistics [commons-math]
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
aherbert merged PR #333:
URL: https://github.com/apache/commons-math/pull/333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Synchronize the remaining mutators of SynchronizedDescriptiveStatistics [commons-math]
Posted by "Lyin-OTTF (via GitHub)" <gi...@apache.org>.
Lyin-OTTF commented on PR #333:
URL: https://github.com/apache/commons-math/pull/333#issuecomment-6281933056
Simplified both to {@inheritDoc} only, pushed in 92d22cb. SynchronizedDescriptiveStatisticsTest (20 tests) still passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Synchronize the remaining mutators of SynchronizedDescriptiveStatistics [commons-math]
Posted by "Lyin-OTTF (via GitHub)" <gi...@apache.org>.
Lyin-OTTF commented on PR #333:
URL: https://github.com/apache/commons-math/pull/333#issuecomment-6291428938
A note on the red CI on df21ddf, so it does not get read as a problem with this change: the failures are pre-existing on master.
This PR sat at action_required for fda0a8a and 92d22cb, so df21ddf is the first commit where the workflows actually ran. The failing tests are all outside the code this PR touches:
- FP64FieldDenseMatrixTest.testVeryLargeAllocation — OutOfMemoryError: Java heap space (the test allocates a very large matrix, so it depends on the runner's heap)
- CorrelatedVectorFactoryTest.testMath226
- CMAESOptimizerTest.testDiagonalRosen, MultiStartMultivariateOptimizerTest.testRosenbrock, LogitTest.testDerivativesWithInverseFunction, UnivariatePeriodicInterpolatorTest.testSine — all off-by-ulp numeric assertions (5.6e-9 / 5.7e-14 against exact 0.0)
master shows the same failures: the Coverage run on master@d3c0eac (https://github.com/apache/commons-math/actions/runs/39334470927) failed on FP64FieldDenseMatrixTest.testVeryLargeAllocation with the same OutOfMemoryError, plus CorrelatedVectorFactoryTest.testMath226 and UnivariatePeriodicInterpolatorTest with the same kind of precision assertion, and the Java CI runs on master@3d15331 failed as well. The summary line of that run is Tests run: 3229, Errors: 1, Flakes: 3 — the surefire flake retry already reruns these.
This change only touches org.apache.commons.math4.legacy.stat.descriptive.SynchronizedDescriptiveStatistics and its test (22 tests, all green locally on Windows 11 / JDK 21). The Java CI run on df21ddf (attempt 2) is still queued; I will follow up here if it reports anything that is actually in stat.descriptive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org