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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
