Repository navigation
Synchronize the remaining mutators of SynchronizedDescriptiveStatistics - #333
Conversation
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.
aherbert
left a comment
aherbert
left a comment
There was a problem hiding this comment.
Thanks for the patch. Please simplify the javadoc for the new synchronized methods.
| /** | ||
| * {@inheritDoc} | ||
| * | ||
| * <p>This method is a mutator like {@link #addValue(double)}, so it has to take the instance lock as well: |
There was a problem hiding this comment.
This comment is not required. Using {@inheritDoc} is fine.
| /** | ||
| * {@inheritDoc} | ||
| * | ||
| * <p>This method is a mutator like {@link #addValue(double)}, so it has to take the instance lock as well: |
There was a problem hiding this comment.
This comment is not required. Using {@inheritDoc} is fine.
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.
|
Simplified both to {@inheritdoc} only, pushed in 92d22cb. SynchronizedDescriptiveStatisticsTest (20 tests) still passes. |
|
This looks good thank you. I note that in the Please also synchronize the |
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>
|
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. |
|
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:
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. |
SynchronizedDescriptiveStatisticsdocuments that "modification and access methods [are] atomic operations for a single instance", but it only overrides twelve of the inherited methods.removeMostRecentValue()andreplaceMostRecentValue(double)are missing, so these two mutators update the storeless statistics without holding the instance monitor. The siblingSynchronizedSummaryStatisticscovers 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 overriddenapply()), 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 withaddValue(), discarding a different element than the caller expects, and with windowed statistics the window bookkeeping can end up inconsistent.DescriptiveStatisticsmethods that mutate state and do not take the lock; everything else (addValue,clear,setWindowSize, thesetXxxImplsetters) already does.Changes:
SynchronizedDescriptiveStatisticsoverrides both mutators withsynchronized, mirroring the existing overrides.SynchronizedDescriptiveStatisticsTestgains three cases:testMutatorsAreSynchronizedreflects over the mutators ofDescriptiveStatisticsand asserts each is declaredsynchronizedin the wrapper, so a mutator added later cannot silently regress;testRemoveMostRecentValueWaitsForInstanceLock/testReplaceMostRecentValueWaitsForInstanceLockhold 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.Verification (Windows, JDK 21):
mvn -pl commons-math-legacy -am test -Dtest=SynchronizedDescriptiveStatisticsTestpasses (20/20, including the 17 inherited cases); reverting onlySynchronizedDescriptiveStatistics.javafails exactly the three new cases.mvn -pl commons-math-legacy checkstyle:checkpasses.