Skip to content

8354522: Clones of DecimalFormat cause interferences when used concurrently #24598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
8 changes: 7 additions & 1 deletion src/java.base/share/classes/java/text/DigitList.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1996, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1996, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -725,7 +725,13 @@ public Object clone() {
char[] newDigits = new char[digits.length];
System.arraycopy(digits, 0, newDigits, 0, digits.length);
other.digits = newDigits;

// data and tempBuilder do not need to be copied because they do
// not carry significant information. They will be recreated on demand.
// Setting them to null is needed to avoid sharing across clones.
other.data = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to copy the data array unlike the digits array (above) because it can just be reset during the next getDataChars call, but a comment as to why might be helpful.

other.tempBuilder = null;

return other;
} catch (CloneNotSupportedException e) {
throw new InternalError(e);
Expand Down
167 changes: 167 additions & 0 deletions test/jdk/java/text/Format/DecimalFormat/CloneTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/*
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test
* @bug 8354522
* @summary Check for cloning interference
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will probably be good to mention somewhere that this test/fix addresses the issue of the same data array reference being shared amongst DigitList clones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added more detail to the comment with the test method.

* @library /test/lib
* @run junit/othervm --add-opens=java.base/java.text=ALL-UNNAMED CloneTest
*/

import jtreg.SkippedException;
import org.junit.jupiter.api.Test;

import java.lang.reflect.Field;
import java.math.BigDecimal;
import java.text.DecimalFormat;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;

import static org.junit.jupiter.api.Assertions.*;

public class CloneTest {

// Note: this is a white-box test that may fail if the implementation is changed
@Test
public void testClone() {
DecimalFormat df = new DecimalFormat("#");
new CloneTester(df).testClone();
}

// Note: this is a white-box test that may fail if the implementation is changed
@Test
public void testCloneAfterInit() {
DecimalFormat df = new DecimalFormat("#");

// This initial use of the formatter initialises its internal state, which could
// subsequently be shared across clones. This is key to reproducing this specific
// issue.
String _ = df.format(Math.PI);
new CloneTester(df).testClone();
}

private static class CloneTester {
private final Field digitListField;
private final Class<?> digitListClass;
private final DecimalFormat original;

public CloneTester(DecimalFormat original) {
this.original = original;
try {
digitListField = DecimalFormat.class.getDeclaredField("digitList");
digitListField.setAccessible(true);

DecimalFormat df = new DecimalFormat();
Object digitList = digitListField.get(df);

digitListClass = digitList.getClass();
} catch (ReflectiveOperationException e) {
throw new SkippedException("reflective access in white-box test failed", e);
}
}

public void testClone() {
try {
DecimalFormat dfClone = (DecimalFormat) original.clone();

Object digits = valFromDigitList(original, "digits");
assertNotSame(digits, valFromDigitList(dfClone, "digits"));


Object data = valFromDigitList(original, "data");
if (data != null) {
assertNotSame(data, valFromDigitList(dfClone, "data"));
}

Object tempBuilder = valFromDigitList(original, "tempBuilder");
if (tempBuilder != null) {
assertNotSame(data, valFromDigitList(dfClone, "data"));
}

assertEquals(digitListField.get(original), digitListField.get(dfClone));
} catch (ReflectiveOperationException e) {
throw new SkippedException("reflective access in white-box test failed", e);
}
}

private Object valFromDigitList(DecimalFormat df, String fieldName) throws ReflectiveOperationException {
Object digitList = digitListField.get(df);
Field field = digitListClass.getDeclaredField(fieldName);
field.setAccessible(true);

return field.get(digitList);
}
}

// Tests that when DecimalFormat is cloned after use with
// a long double/BigDecimal, clones will be independent. This is not an
// exhaustive test. This tests for the issue of the same DigitList.data
// array being reused across clones of DecimalFormat.

@Test
public void testCloneIndependence() {
AtomicInteger mismatchCount = new AtomicInteger(0);
DecimalFormat df = new DecimalFormat("#");
CountDownLatch startSignal = new CountDownLatch(1);

// This initial use of the formatter initialises its internal state, which could
// subsequently be shared across clones. This is key to reproducing this specific
// issue.
String _ = df.format(Math.PI);

try (var ex = Executors.newThreadPerTaskExecutor(Thread.ofPlatform().factory())) {
for (int i = 0; i < 5; i++) {
final int finalI = i;
// Each thread gets its own clone of df
DecimalFormat threadDf = (DecimalFormat) df.clone();
Runnable task = () -> {
try {
startSignal.await();
for (int j = 0; j < 1_000; j++) {
if (mismatchCount.get() > 0) {
// Exit early if mismatch has already occurred
break;
}

int value = finalI * j;
String dfString = threadDf.format(BigDecimal.valueOf(value));
String str = String.valueOf(value);
if (!str.equals(dfString)) {
mismatchCount.getAndIncrement();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mismatchCount is no longer needed. Simply break after printing the error message would suffice.

Copy link
Contributor Author

@j3graham j3graham Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still need something to indicate that it had failed, as well as a way to signal other threads that they should terminate early. Worth changing to an AtomicBoolean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok then. Thanks.

Copy link
Contributor Author

@j3graham j3graham Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to leave as is? Or is the AtomicBoolean more clear?

Copy link
Member

@naotoj naotoj Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to use it as a failure indicator (still I'd suggest breaking immediately after printing error)

System.err.println("mismatch: str = " + str + " dfString = " + dfString);
break;
}
}
} catch (InterruptedException e) {
Copy link
Member

@naotoj naotoj Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest throwing the exception (or RuntimeException with it as the cause), not swallowing it silently in the test.

throw new RuntimeException(e);
}
};
ex.execute(task);
}
startSignal.countDown(); // let all tasks start working at the same time
}
assertEquals(0, mismatchCount.get());
}
}