ICU-23403 Fix data race in CurrencyPrecision.withCurrency

[Manishearth]

Clone the Precision object returned by constructFromCurrency before mutating its trailingZeroDisplay field. This prevents mutating shared static instances like FIXED_FRAC_0 or FIXED_FRAC_2 when multiple threads concurrently call withCurrency.

This was investigated and fixed by Gemini.

Checklist

• Required: Issue filed: /ICU-23403
• Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable
• Approver: Feel free to merge on my behalf

[jira-pull-request-webhook]

Notice: the branch changed across the force-push!

• icu4j/main/common_tests/src/test/java/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java is no longer changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[Manishearth]

I'm not sure how to best test this.

Gemini generated the following complicated test

Details

    @Test
    public void testCurrencyPrecisionConcurrency() throws InterruptedException {
        final Currency usd = Currency.getInstance("USD"); // 2 decimals (maps to FIXED_FRAC_2)
        
        final CurrencyPrecision cp1 = Precision.currency(CurrencyUsage.STANDARD)
                .trailingZeroDisplay(TrailingZeroDisplay.HIDE_IF_WHOLE);
        final CurrencyPrecision cp2 = Precision.currency(CurrencyUsage.STANDARD)
                .trailingZeroDisplay(TrailingZeroDisplay.AUTO);
        int numThreads = 10;
        final int iterations = 5000;
        Thread[] threads = new Thread[numThreads];
        final java.util.concurrent.atomic.AtomicInteger failures = new java.util.concurrent.atomic.AtomicInteger(0);
        
        for (int i = 0; i < numThreads; i++) {
            final int threadId = i;
            threads[i] = new Thread(new Runnable() {
                @Override
                public void run() {
                    for (int j = 0; j < iterations; j++) {
                        if (threadId % 2 == 0) {
                            Precision p = cp1.withCurrency(usd);
                            String formatted = NumberFormatter.with()
                                    .precision(p)
                                    .locale(Locale.US)
                                    .unit(usd)
                                    .format(123.00)
                                    .toString();
                            // USD standard format for 123.00 is "$123.00".
                            // If HIDE_IF_WHOLE is working, it should be "$123".
                            if (!formatted.equals("$123")) {
                                failures.incrementAndGet();
                            }
                        } else {
                            Precision p = cp2.withCurrency(usd);
                            String formatted = NumberFormatter.with()
                                    .precision(p)
                                    .locale(Locale.US)
                                    .unit(usd)
                                    .format(123.00)
                                    .toString();
                            // If AUTO is working, it should be "$123.00".
                            if (!formatted.equals("$123.00")) {
                                failures.incrementAndGet();
                            }
                        }
                    }
                }
            });
        }
        for (Thread t : threads) {
            t.start();
        }
        for (Thread t : threads) {
            t.join();
        }
        
        assertEquals("Concurrently configured precisions should not corrupt each other's formatting behavior", 0, failures.get());
    }

[markusicu]

Thanks! Discussion:

• the fix looks good for this call site, but...
• we do need a test; should be simple, something like
  • format something using the cached Precision as is
  • format something in a way that modifies the Precision
  • format like the first time again -- with the bug it gives the wrong result
• something like withTrailingDisplay() would help with performance
• the real problem is caching a mutable object
• we would like to solve that, for a robust solution
• @sffc we would like your help here
• we assume that this is not a problem in C++ because we use const there which would force us to make a copy before modifying; someone should verify
• Shane: why is Precision mutable, at least in Java?
• if we don't want to radically change the design, consider making Precision Freezable, and freeze() the cached instances?

[sffc]

This was added together with C++ in b79c299

The C++ code appears to use the copy constructor; it is different than Java because it stores the settings locally in a struct instead of retaining static references to things.

[sffc]

the real problem is caching a mutable object

This is not intended to be a mutable object. The problem is code setting a private field at a distance.

[sffc]

if we don't want to radically change the design, consider making Precision Freezable, and freeze() the cached instances?

This helps nothing. Freezable is an ICU4J abstraction that relies on code implementing it correctly. It does not protect against code from somewhere else in the program setting private fields.

[sffc]

You don't need to look far to see that the field is designed to be private and immutable. I don't remember the reason they are package-private instead of private, and why they are not final.

public abstract class Precision {

    /* package-private final */ MathContext mathContext;
    /* package-private final */ TrailingZeroDisplay trailingZeroDisplay;

[jira-pull-request-webhook]

Notice: the branch changed across the force-push!

• icu4j/main/core/src/main/java/com/ibm/icu/number/CurrencyPrecision.java is different
• icu4j/main/core/src/main/java/com/ibm/icu/number/Precision.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[Manishearth]

Tried to add a simple test. We already had a withTrailingDisplay-equivalent method so I reused that.

[jira-pull-request-webhook]

Notice: the branch changed across the force-push!

• icu4j/main/common_tests/src/test/java/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

[Manishearth]

the LLM noticed this null-auto correspondence, I verified it, btu I'm not 100% sure, this isn't my codebase

[Manishearth]

The test won't test against the concurrency issue but it ought to test against the underlying bug. Good enough.