Java.math.BigDecimal toString is not thread safe • Hacker News Board

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

#81

Post by xxs » Wed, Aug 30, 2017, 8:46 PM UTC

pulse7 wrote: post #17

Very similar bug was in Digitalk Smalltalk 25 years ago: conversion from integer to string used a global cache... I notices this while running a batch with progress indicator on UI. Sometimes integer to string conversion in batch didn't work and I couldn't find the reason. After loooooooong period of extensive debugging the issue was pinned down to global cache in integer to string conversion, which was not thread sa…

BigDecimal has nothing to do with the issue. There is no race conditional. The code is textbook example how to use (cheap) unsafe publishing with no locks/CAS -- assign to local variable and 'return' the variable value (not a class member one). There is no global cache, it's per BigDecimal instance. As for 'this bug', yes JVM concurrency bugs are serious by definition.

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

#82

Post by alkonaut » Wed, Aug 30, 2017, 9:30 PM UTC

andars wrote: post #49

readittwice wrote: post #43

Earlier quoted context omitted.

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.

Exactly. Relevant passage from ^

    An object is considered to be completely initialized when its constructor finishes. 
    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.

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

#83

Post by MichaelGG » Wed, Aug 30, 2017, 9:57 PM UTC

aardvark179 wrote: post #35

MichaelGG wrote: post #32

Earlier quoted context omitted.

It could cause other correctness issues that could break CAS, for instance (which yeah seems like a poor idea in hindsight since it's so complicated). If this is actually allowed it seems like a huge mistake in the .NET memory model. I've never seen a definitive answer on it (not saying you're wrong).

It has to be legal for the VM to store the object before it has been entirely initialised, but the question is whether other threads will be able to see it in that uninitialised state. If other threads can see a partially initialised object then you remove most of the nice guarantees final fields give the JIT. NB. You can break all the JMM guarantees by having the constructor share the object reference with another t…

If you do it from the constructor (assign this to a shared field) then that seems acceptable and narrows the scope of issues a ton. Do you know of a definitive answer on this by an actual team member with citations? I've seen well respected community people assert that partially constructed objects can be visible but that seems wrong. I would expect this to have practical memory safety issues: somewhere in the standard libraries there have to be interoperability classes that could result in unsafe access if partially constructed.

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

#84

Post by hyperpape » Thu, Aug 31, 2017, 12:26 AM UTC

Someone wrote: post #79

juancn wrote: post #67

Earlier quoted context omitted.

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…

Immutable implies thread safe. Somewhere in the concurrency section of Effective Java, it both that this is true and that documenting a class as immutable is good enough to document that it's thread safe.

Not sure if the spec lays it out, but it's a consequence of the spec.

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

#85

Post by vivaamerica1 » Thu, Aug 31, 2017, 2:25 AM UTC

Agree with others that String.length() should never ever throw NPE from within String, regardless of it being called from whatever thread.

Now, in the comments there's another idea suggesting the unsafe publication of String (partially constructed). This would be the case if String.value wasn't final (essentially the case of double-checked locking). But that's not the case here because String.value is final[0]

While it's a JVM bug, making stringCache volatile would be one way to fix it - unless the JVM is also broken around volatile...

[0] https://stackoverflow.com/questions/11306032/please-explain-...

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

#86

Post by johncolanduoni » Thu, Aug 31, 2017, 7:33 AM UTC

lostmsu wrote: post #58

johncolanduoni wrote: post #25

Earlier quoted context omitted.

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.

I doubt that statement is correct.

It is, Jon Skeet (as usual) has a great blog post on it that even discusses Java: http://csharpindepth.com/Articles/General/Singleton.aspx

Specifically the "third version" exposes the object before the effects of construction are necessarily visible on other threads.

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

#87

Post by aardvark179 » Thu, Aug 31, 2017, 7:43 AM UTC

MichaelGG wrote: post #83

aardvark179 wrote: post #35

Earlier quoted context omitted.

It has to be legal for the VM to store the object before it has been entirely initialised, but the question is whether other threads will be able to see it in that uninitialised state. If other threads can see a partially initialised object then you remove most of the nice guarantees final fields give the JIT. NB. You can break all the JMM guarantees by having the constructor share the object reference with another t…

If you do it from the constructor (assign this to a shared field) then that seems acceptable and narrows the scope of issues a ton. Do you know of a definitive answer on this by an actual team member with citations? I've seen well respected community people assert that partially constructed objects can be visible but that seems wrong. I would expect this to have practical memory safety issues: somewhere in the standa…

The safety guarantees only apply to final fields, it's entirely possible to see non-final fields in a half initialized state, and even half written longs because the memory model does not guarantee those operations are atomic. To be safe in those cases you must use synchronization or something else that establishes a happens before relationship.

I think there's some good talks on the JMM on YouTube, and stuff on Shipilev"s blog.

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

#88

Post by johncolanduoni » Thu, Aug 31, 2017, 7:48 AM UTC

jmull wrote: post #55

johncolanduoni wrote: post #25

Earlier quoted context omitted.

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.

That seems like a bad thing. I'd think that you'd want your language to guarantee an initializer has completed before letting an object/structure/whatever be visible. At least as the default. I mean, that doesn't even give an object a chance to initialize a synchronization mechanism before it's thrown into the wild where it needs one. It sounds like initializing fields to zero is guaranteed, but is that flexible enou…

It's a performance trade off, because in general it's hard for the compiler to know whether it needs to insert a memory barrier or not if you do something like:

    this.fooField = new Something()

which would need a memory barrier if another thread is expected to access fooField concurrently, and not need one otherwise. Putting memory barriers on every field assignment of a constructed object would be pretty expensive.

It's also worth noting there's not anything that special about constructors here; there's a million other ways you can get similar issues when you try to handle concurrency at this level. For example if you do:

    this.bar.changeSomeFields()
    this.fooField = this.bar

and changeSomeFields doesn't invoke a memory barrier at the right time you'll get the same problem if fooField isn't volatile.

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

#89

Post by readittwice » Thu, Aug 31, 2017, 8:30 AM UTC

andars wrote: post #49

readittwice wrote: post #43

Earlier quoted context omitted.

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.

Thanks, that explains it! Didn't know that final fields have this special rule and that the value field in the String class is final.

So to recap: this shouldn't be possible for Strings, since the value field in the String class is final and therefore has a memory barrier. But it could happen for other classes with non-final fields.

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

#90

Post by jmull » Fri, Sep 01, 2017, 10:32 PM UTC

johncolanduoni wrote: post #88

jmull wrote: post #55

Earlier quoted context omitted.

That seems like a bad thing. I'd think that you'd want your language to guarantee an initializer has completed before letting an object/structure/whatever be visible. At least as the default. I mean, that doesn't even give an object a chance to initialize a synchronization mechanism before it's thrown into the wild where it needs one. It sounds like initializing fields to zero is guaranteed, but is that flexible enou…

It's a performance trade off, because in general it's hard for the compiler to know whether it needs to insert a memory barrier or not if you do something like: this.fooField = new Something() which would need a memory barrier if another thread is expected to access fooField concurrently, and not need one otherwise. Putting memory barriers on every field assignment of a constructed object would be pretty expensive. I…

Yes, I guess it is reasonable.

Come to think of it, I objected because I thought it doesn't give you a chance to initialize whatever your object/structure/whatever's synchronization mechanism is. But that's not really true. The call to initialize your synchronization mechanism would contain the barrier you need so a general initialization doesn't generally need one.