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

71–80 of 90 posts

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

#71

Post by pvg »

LgWoodenBadger wrote: post #68
pvg wrote: post #53

Earlier quoted context omitted.

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…

It has nothing to do with whether String is correct

It has everything to do with it. You have to explain how you ended up with a broken instance of String. The internal state of a String instance is hosed, which is a massive violation of all sorts of JVM guarantees and assumptions - this is the OG immutable class, after all. There are two options - a broken String implementation or a broken JVM implementation.

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

#72

Post by LgWoodenBadger »

mmastrac wrote: post #56
LgWoodenBadger wrote: post #51

Earlier quoted context omitted.

You'd have to point out specifics you're mentioning, but the read of stringCache is not through a final field, and that's the source of the problem for any thread that did not initialize or otherwise set stringCache to a value.

The JLS specifically points out string's final fields as being important to the memory model for security purposes. This is a JVM implementation bug. Quote: >String objects are intended to be immutable and string operations do not perform synchronization. While the String implementation does not have any data races, other code could have data races involving the use of String objects, and the memory model makes weak…

This is only true if you have a safe reference to that String. No safe reference has been established between stringCache and the String instance it points to.

Assuming you have a safe reference (through a safe publication), then what you quoted comes into play. You do not need any synchronization mechanisms around a String in order to see its correct data, because the String instance is immutable.

Or to put it another way, why do you think other threads would see the value of stringCache change from

    stringCache = "this is my value in thread 1.";
to

    stringCache = "this is a different value set by another thread without a safe publication of the stringCache reference.";
without establishing a happens-before relationship through a memory barrier?

You wouldn't. That's why there are AtomicLong, AtomicBoolean, and AtomicInteger. Long, Boolean, and Integer are all immutable, but you can't use them without establishing happens-before. AtomicReference could have been used to get the correct behavior intended by stringCache, but AtomicReference didn't exist until 1.5.

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

#73

Post by GreenToad »

In java lang specs: "A thread that can only see a reference to an object after that object has been completely initialized is guaranteed to see the correctly initialized values for that object's final fields." This makes it a JVM bug because no thread should be able to see uninitialized final fields of an object.

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

#74

Post by cesarb »

LgWoodenBadger wrote: post #52
pvg wrote: post #45

Earlier quoted context omitted.

If you look at the trace, the failure is an NPE in String. No amount of 'thread unsafety' in a caller should cause String.length() to NPE out.

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.

> If you have a reference to a junk instance

Step back a bit: where did the junk instance come from? The Java rules guarantee that a String created through the String constructors is valid on any thread that can see it. There are no "junk instances" of String. Also, writing to an object reference variable is indivisible: either a thread doesn't see the write, or it sees the full write.

So there are only two states in which a thread can see the stringCache field: either it is null, or it has a reference to a valid String. It might not see the field write from another thread, but in this case this only leads to duplicating work and leaving a bit of garbage on the heap for the GC to clean.

(Of course, once you start using reflection to manipulate internal fields of an object, then all bets are off, but that's not the case here.)

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

#75

Post by cesarb »

LgWoodenBadger wrote: post #72
mmastrac wrote: post #56

Earlier quoted context omitted.

The JLS specifically points out string's final fields as being important to the memory model for security purposes. This is a JVM implementation bug. Quote: >String objects are intended to be immutable and string operations do not perform synchronization. While the String implementation does not have any data races, other code could have data races involving the use of String objects, and the memory model makes weak…

This is only true if you have a safe reference to that String. No safe reference has been established between stringCache and the String instance it points to. Assuming you have a safe reference (through a safe publication), then what you quoted comes into play. You do not need any synchronization mechanisms around a String in order to see its correct data, because the String instance is immutable. Or to put it anoth…

> Or to put it another way, why do you think other threads would see the value of stringCache change

True, other threads might not see the value of stringCache change. However, if and when they see it change, they will see it change to another valid String. They will never see an incompletely initialized String.

Using AtomicReference or volatile gives you stronger guarantees, but they're not necessary here.

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

#76

Post by xxs »

Not sure if late for the party... But there is no bug int the impl. BigDecimal.toString(). It's a JVM bug. It blatantly violates the spec of the original JMM (as of Java 1.5 and backported to 1.4)

Not only that but it lacks optimization/intrinsics for String. String.length() should never/ever yield a NPE. I'd expect a process crash than adding metadata for trapping read access faults.

---

Edit: final fields have become ubiquitous even for objects that are safely published [via volatile, synchronizeed, CAS, before thread.start()]. All wrapper classes Integer, Long, etc. have them. Final fields are very much advised to be used and they do help reliability and readability by making it easy to reason about object state (i.e. it doesn't change once seen). On x86 field fields require a compiler barrier at best as the writes are not reordered. In other words they are extremely cheap. Stuff like AtomicReference.lazySet is next to free and a welcome way to build fast concurrency primitives.

There is Doug Lea's parer[0] for the improved jmm. There are different versions of ARM architecture with different memory models, overall ARM is considered weak. v7 has dmb[1] only, and to my knowledge it's not cheap. Skipping dmb requires rather deep analysis, so I wonder if that was the case experienced by the poster. ARMv8 has store-release fence but I don't know how efficient would be spamming it.

0: http://gee.cs.oswego.edu/dl/html/j9mm.html

1: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc....

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

#77

Post by xxs »

charleslmunger wrote: post #37
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 believe the guarantees around final fields were only added in java 7 - possibly the author is using something older?

Since 1.5 (and backported to 1.4)

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

#78

Post by xxs »

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…

stringCache is a red herring. While it publishes the string result unsafely, any string should be safe for use at any thread.

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

#79

Post by Someone »

juancn wrote: post #67
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.

That's what my question is about. The OP claims "immutable implies thread-safe". My question is whether the spec promises that, and if so, where.

Reading https://docs.oracle.com/javase/7/docs/api/java/math/BigDecim..., I can see the class is immutable, but that page mentions neither "thread" nor "concurrent".

And yes, I think it has to define what it means by those terms before one can assume that seemingly obvious claim to be true. Reason? Both thread-safe and immutable are fairly vague terms that different readers can interpret differently. For example, BigDecimal is declared to be immutable, but, in the implementation being discussed, has a field that can get modified when one calls toString() on it.

Post reply on HN