Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Java.­math.­BigDecimal toString is not thread safe

vmlens.com

Post reply on HN
Print view View original

61–70 of 90 posts

Re: Java.­math.­BigDecimal toString is not thread safe

#61

Post by juancn »

readittwice wrote: post #13
TazeTSchnitzel wrote: post #6

I don't understand the race condition here. stringCache is only ever assigned to with an initialised string (the output of stringCache()), so given Strings are immutable and assuming Java's assignment is atomic, how can you get a race condition that causes a problem? There shouldn't be a TOCTTOU issue either given stringCache is copied to sc before being null-checked. What is the statement reordering doing here to br…

AFAIU the JIT-compiler (or the CPU) could reorder the code: String x = allocate String; // allocate memory only (no initialization) initialize string x // constructor publish x // e.g. store in some global reordered to String x = allocate String; publish x // after publishing some other thread could read unintialized object initialize string x

It cannot do that, value is final, so value has to be fully initialized before publishing. The only way that could happen is if the String constructor unsafely published the string (which is not the case)

Re: Java.­math.­BigDecimal toString is not thread safe

#62

Post by juancn »

joncrocks wrote: post #9
TazeTSchnitzel wrote: post #6

I don't understand the race condition here. stringCache is only ever assigned to with an initialised string (the output of stringCache()), so given Strings are immutable and assuming Java's assignment is atomic, how can you get a race condition that causes a problem? There shouldn't be a TOCTTOU issue either given stringCache is copied to sc before being null-checked. What is the statement reordering doing here to br…

The issue may be to do with the guarantees that the JVM gives you about orders of operations and write visibility across threads. The author hints at a possible solution when they say "As we see a non-volatile field stringCache", the key being 'volatile' which is a Java keyword with a specific meaning that constrains the order of operations and write visibility across threads. See http://tutorials.jenkov.com/java-con…

It's a bug in the JVM, value is final and assigned in the constructor after the array is fully initialized, so it's impossible as per the JMM to see a partially initialized string. My guess is that the RaspPi VM is buggy.

Re: Java.­math.­BigDecimal toString is not thread safe

#63

Post by juancn »

hyperpape wrote: post #8
TazeTSchnitzel wrote: post #6

I don't understand the race condition here. stringCache is only ever assigned to with an initialised string (the output of stringCache()), so given Strings are immutable and assuming Java's assignment is atomic, how can you get a race condition that causes a problem? There shouldn't be a TOCTTOU issue either given stringCache is copied to sc before being null-checked. What is the statement reordering doing here to br…

You can see a partially constructed String is the problem. Assignment is atomic, but it's not guaranteed to not be reordered with operations done in the constructor. The double checked locking is broken declaration describes this: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedL... Edit: nevermind, I believe others' points about Strings having a final value make this irrelevant.

You cannot see a partially constructed String in this case, there are very string visibility guarantees in the JMM post JSR-133. This is a JVM bug.

Re: Java.­math.­BigDecimal toString is not thread safe

#64

Post by juancn »

LgWoodenBadger wrote: post #44
hyperpape wrote: post #8

Earlier quoted context omitted.

You can see a partially constructed String is the problem. Assignment is atomic, but it's not guaranteed to not be reordered with operations done in the constructor. The double checked locking is broken declaration describes this: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedL... Edit: nevermind, I believe others' points about Strings having a final value make this irrelevant.

I disagree that the others' points about Strings having final values make this irrelevant. It's the references that must be final in order for things to be irrelevant, but stringCache (nor the other instance-level variables used by layoutChars() such as scale, intCompact, etc.) is not final. It's just transient, which doesn't affect anything with respect to thread-safe behavior. If stringCache was final then this pro…

At most, per the JMM this can only initialize the cached string in BigDecimal more than once, but you can never see a partially initialized string. This is a JVM bug.

Re: Java.­math.­BigDecimal toString is not thread safe

#65

Post by hyperpape »

cesarb wrote: post #57
hyperpape wrote: post #26

Earlier quoted context omitted.

It's immutable, and immutable objects are thread safe. This is also true in C: you can safely pass references to structs between threads with no locks, as long as you never mutate them. Java just enforces that immutability if you write the class the right way. ...the bug is that it's not really immutable, insofar as it's using an unsafe mutation under the hood to implement its toString method. So it either needs to d…

The rules for C and Java are different. In Java, it's guaranteed that the assignments in the constructor to fields marked as final is visible to other threads before the newly constructed object is (unless you stash a reference to it before returning to the constructor), but C has no such guarantee, so in C you have to put the memory barriers yourself.

Yes, what Java gives you for immutable objects is a guarantee of safe publication. In both languages, once you have the object published, you can freely share it between threads.

Re: Java.­math.­BigDecimal toString is not thread safe

#66

Post by hyperpape »

juancn wrote: post #63
hyperpape wrote: post #8

Earlier quoted context omitted.

You can see a partially constructed String is the problem. Assignment is atomic, but it's not guaranteed to not be reordered with operations done in the constructor. The double checked locking is broken declaration describes this: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedL... Edit: nevermind, I believe others' points about Strings having a final value make this irrelevant.

You cannot see a partially constructed String in this case, there are very string visibility guarantees in the JMM post JSR-133. This is a JVM bug.

Yes, that you can't see a partially constructed String is what the edit is saying, but I probably could've been more explicit.

Re: Java.­math.­BigDecimal toString is not thread safe

#67

Post by juancn »

Someone wrote: post #48

"BigDecimal is an immutable data type. So every method of this class should be thread safe." I'm not familiar with the ins and outs of Java's API and memory model and this is unexpected, but should it? If so, where does the documentation make that promise? Also (nitpick) one _can_ subclass BigDecimal and make it mutable (a design error that won't be fixed because of backward compatibility)

This is a JVM bug, BigDecimal is thread safe, but there's peculiarity of the java memory model that the ARM JVM is not honoring and thus the bug.

Re: Java.­math.­BigDecimal toString is not thread safe

#68

Post by LgWoodenBadger »

pvg wrote: post #53
LgWoodenBadger wrote: post #52

Earlier quoted context omitted.

If you have a reference to a junk instance, which is what can happen with an unsafe publication (which is what is going on with the stringCache member variable) then all bets are off in terms of what you can expect to see.

Where is the 'unsafe publication' of String.value that you see? If you can create a 'junk instance' of String, your problem is with String.

This is the implementation of java.math.BigDecimal from JDK 1.6.0_45

    private transient String stringCache;

    public String toString() {
        String sc = stringCache;
        if (sc == null)
            stringCache = sc = layoutChars(true);
        return sc;
    }
Within a thread that sees "sc == null" the assignment of stringCache and sc to the result of layoutChars(true); will be legitimate (they will see a valid, fully-formed, correct String).

Another thread calling toString() may or may not see sc, or stringCache, assigned to a legitimate String instance, due to the Java Memory Model (happens-before, memory barriers, CPU implementation details, cache levels, etc.).

The data within the String created by layoutChars will be consistent to anybody who has established a happens-before relationship with respect to stringCache (either the thread who created it, or if stringCache was volatile, or if stringCache was final, or if a memory-barrier (synchronized, atomic primitive, etc.) has been erected around it.

None of that is present in this implementation of BigDecimal.toString(), which is why stringCache (and the vars used in layoutChars()) is not a valid publication in the Java Memory Model.

It has nothing to do with whether String is correct, and it's not a defect in the JRE implementation. No safe publication was established with respect to stringCache, therefore the data that other threads see is undefined.

Re: Java.­math.­BigDecimal toString is not thread safe

#69

Post by juancn »

johncolanduoni wrote: post #25
aardvark179 wrote: post #22

Earlier quoted context omitted.

Looking at the article I see he's running on a raspberry pi. I would guess the problem is that the openjdk aarch32 jit is not correctly implementing the java memory model as final fields should have been initialised before an object is visible in another thread. So it's not really that BigDecimal isn't thread safe, but that there's a bug in the JIT.

I'm not sure if this is the case for the JVM, but for .NET it is perfectly legal for the VM to store the object address somewhere after allocation but prior to the constructor running/becoming visible. All of the fields objects will be zeroed so this won't cause memory unsafety, but it's rarely what you expect to happen. Forgetting this is a big source of bugs in double checked locking in .NET.

Java is the same, but final fields are special. The object cannot be published until they're completely initialized (unless the constructor unsafely publishes them).

Re: Java.­math.­BigDecimal toString is not thread safe

#70

Post by juancn »

LgWoodenBadger wrote: post #42
needusername wrote: post #15

Earlier quoted context omitted.

Yes but String.value is final and initialized in the constructor. So it should be safely published.

But the stringCache reference is not final, it's just transient. Because there is nothing within the toString() implementation that enforces a happens-before effect, nor is there anything similar on stringCache itself (such as volatile), threads are not guaranteed to see anything correctly with respect to stringCache, or the layoutChars() method. The problems in layoutChars also affect the thread-safe behavior of toE…

It's irrelevant, the bug is that the JVM published the String before String.value was completely initialized. This is in violation of the JMM.
Post reply on HN