[insaf021]

https://issues.apache.org/jira/browse/MATH-1694

PolynomialsUtils.createJacobiPolynomial races on its static coefficient cache. Its double-checked locking on a plain HashMap publishes the list for a new (v, w) before seeding it, and buildPolynomial then locks the list rather than JACOBI_COEFFICIENTS, so a racing caller extends a partially seeded list. With 16 threads per fresh key, 335 of 32000 calls returned a wrong polynomial (P_3^(1,0)(1) = 5.8, expected 4) and 29 threw.

JACOBI_COEFFICIENTS is now a ConcurrentHashMap and the list is seeded inside computeIfAbsent, so it's only published once complete and lookups of existing keys don't contend. buildPolynomial copies the coefficients into the double[] before releasing the list lock, as another thread may be growing the same ArrayList; the PolynomialFunction is built outside the lock.

testJacobiConcurrentCreation failed in 3 of 3 runs on master and passes with the patch.

The BaseRuleFactory.getRule race is tracked separately as MATH-1693.

[aherbert]

The suggested updates will minimize locking duration for concurrent threads.

[aherbert]

Since this is using a Map this is more explicit in its support of concurrency using a ConcurrentHashMap:

        JACOBI_COEFFICIENTS = new ConcurrentHashMap<>();

        // ...

        List<BigFraction> coefficients = JACOBI_COEFFICIENTS.computeIfAbsent(key, k -> {
            // allocate a new list for v, w
            final List<BigFraction> list = new ArrayList<>();

            // Pv,w,0(x) = 1;
            list.add(BigFraction.ONE);

            // P1(x) = (v - w) / 2 + (2 + v + w) * X / 2
            list.add(BigFraction.of(v - w, 2));
            list.add(BigFraction.of(2 + v + w, 2));

            return list;
        });

Using a ConcurrentHashMap will not lock the entire table allowing other concurrent threads to obtain coefficients that exist.

[insaf021]

Done, switched JACOBI_COEFFICIENTS to a ConcurrentHashMap and seed the list inside computeIfAbsent. Lookups for existing keys no longer contend with a new key being seeded. The HashMap import went with it.

[aherbert]

building the polynomial does not have to be synchronized once the coefficients to the required degreee are created:

final double[] a;
synchronized (coefficients) {
    // ...
}
// build the polynomial
return new PolynomialFunction(a);

[insaf021]

Agreed, only the growth and the copy into the double[] need the list lock. Moved the PolynomialFunction construction outside the synchronized block. PolynomialsUtilsTest, including the concurrent test, still passes.

[aherbert]

Thanks for the modifications. Note that these fixes now have associated Jira tickets: MATH-1693 (BaseRuleFactory) and MATH-1694 (PolynomialUtils).

Can you remove the modification of BaseRuleFactory in this PR so it corresponds only to MATH-1694. Then create a new PR for MATH-1693 with the single method change to use synchronized. That requires a test. The description for this noted that the existing code can spin indefinitely. A test can be created using JUnit 5 Assertions.assertTimeoutPreemptively to show the block of functionality executes within a time frame. The test runner can handle JUnit 4 and JUnit 5 mixed in the same test class.

[insaf021]

Done, the BaseRuleFactory change is gone so this is MATH-1694 only now, and I've retitled it to match. PolynomialsUtilsTest still passes.

MATH-1693 will go in its own PR with the synchronized change and an assertTimeoutPreemptively test.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.22%. Comparing base (f554608) to head (618cf4d).
⚠️ Report is 398 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #340      +/-   ##
============================================
+ Coverage     86.54%   87.22%   +0.68%     
+ Complexity     9787       91    -9696     
============================================
  Files           532      499      -33     
  Lines         35516    33530    -1986     
  Branches       6194     5851     -343     
============================================
- Hits          30738    29248    -1490     
+ Misses         3518     3164     -354     
+ Partials       1260     1118     -142     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.