Live data from Hacker News

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

vmlens.com

41–50 of 90 posts

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

#41
post #38
post #16

Earlier quoted context omitted.

> the key being 'volatile' which is a Java keyword with a specific meaning that constrains the order of operations and write visibility across threads. I must point out that "volatile" in Java means something completely different than what it means in C and related languages. In multithreaded C code, "volatile" is almost always incorrect. There are only a few correct and portable uses of volatile (such as dealing wit…

The most important thing being that in Java, volatile is a memory barrier, whereas in C it isn't. One thing I'm unsure of: I think in C, a volatile write to some location and a volatile write to another location (even without any data dependency) may not be reordered); is this correct?

C's volatile guarantees very little. It's mostly there for compilers to implement in whatever way is useful for the environment they run in, and so there isn't much portable code you can usefully write using volatile.

All it really guarantees is that reads and writes won't be elided. For example:

  *x = 42;
  *x = 43;
If x is a normal pointer, the compiler can eliminate the first line. If it's a pointer to volatile, the compiler must write both values.

Volatile predates multithreading in C (it was meant for memory-mapped IO and similar things) and hasn't been updated for it, so it has pretty much no useful properties for multithreading. There are no guarantees about reordering when it comes to multiple threads. You're guaranteed to see reads and writes in the correct order from the perspective of the thread your code is running on, but the compiler won't insert any memory barriers, so it's completely up for grabs how other threads might see it.

(More completely, it depends entirely on your CPU's memory model. If you're on an architecture which does strict ordering at the hardware level then you could potentially take advantage of that. If you aren't then you'll see whatever crazy results hardware reordering might produce.)

In contrast, Java's volatile is only about multi-threading. So really, the only thing that's similar between the two languages' use of volatile is how they spell the keyword.

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

#42
post #7

Earlier quoted context omitted.

Using the Java terminology, there is no "happens-before" edge. So every action must appear as if it occurred in the specified order in that thread, but other threads are free to see those actions in a different order. On some machines/CPUs/architectures, this is very easy to organise, since the writes can and will be re-ordered. When this happens you can get the pointer to the string being set, before the contents of…

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 toEngineeringString() as well.

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

#43

Earlier quoted context omitted.

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 could, but as far as I know it then isn't a compliant JIT any more. That is, it's either a bug or a deliberate deviation from the specification.

Why is the JIT not allowed to do this? (Assuming publishing isn't volatile) The reordering could also be caused by the CPU, not only by the JIT. If the JIT isn't allowed to reorder, we also need to prevent the CPU from reordering. Otherwise it doesn't make sense to restrict the JIT from reordering if the CPU could still reorder later. We would need to emit memory barriers for non-volatile loads & stores to prevent reordering on architectures like ARM. This sounds quite expensive.

This doesn't make sense to me. Am I thinking wrong? Can you point me to documentation/reference that states that the JIT isn't allowed to do this?

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

#44

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.

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 problem wouldn't exist. Neither would it exist if stringCache was volatile (though you would potentially duplicate work).

The implementation of toString() (specifically the assignment of stringCache) is a perfect example of an unsafe publication in the Java Memory Model world.

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

#45

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…

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.

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

#47

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…

How do you read the JLS bits about final fields that mmastrac referenced then? https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.htm...

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

#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)

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

#49

Earlier quoted context omitted.

It could, but as far as I know it then isn't a compliant JIT any more. That is, it's either a bug or a deliberate deviation from the specification.

Why is the JIT not allowed to do this? (Assuming publishing isn't volatile) The reordering could also be caused by the CPU, not only by the JIT. If the JIT isn't allowed to reorder, we also need to prevent the CPU from reordering. Otherwise it doesn't make sense to restrict the JIT from reordering if the CPU could still reorder later. We would need to emit memory barriers for non-volatile loads & stores to prevent re…

See section 17.5, "final Field Semantics" in http://docs.oracle.com/javase/specs/jls/se8/jls8.pdf.

Note that a String's value field is final.

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

#50

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?

Nope, the current JMM dates from 2004.
Post reply on HN