11 ms·

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 th

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 the string are actually set. Within the thread, you won't see this, since reorders within the thread of context are done "safely", or at least are visible, but to a different thread/core/CPU those writes may arrive out of order.

needusername 9y ago
Yes but String.value is final and initialized in the constructor. So it should be safely published.
- aardvark179 9y ago
  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.
  - mmastrac 9y ago
    I agree. The Java memory model explicitly stated that final fields will be correctly initialized in any object reference visible from another thread, provided a reference to that object is only stored after the constructor completes.
  - johncolanduoni 9y ago
    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.
    - MichaelGG 9y ago
      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).
      - aardvark179 9y ago
        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 thread. The JMM even says: > If a reference to an object is shared with other threads during the initial construction of an object, most of the guarantees for final fields of that object can go kerflooey; this includes cases in which other parts of a program continue to use the original value of this field.
        MichaelGG 9y ago
        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.
        aardvark179 9y ago
        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.
  - needusername 9y ago
    That would be consistent with my understanding of the JMM and unfortunately my prejudices against "community maintained" backends and ports of OpenJDK. I would not be surprised if this surfaces on other places as well, AFAIK the JDK is full of such benign data races: https://www.youtube.com/watch?v=UykhZ36W04I&index=13&list=PLX8CzqL3ArzXJ2EGftrmz4SzS6NRr6p2n https://www.youtube.com/watch?v=UykhZ36W04I&index=13&list=PL...
  - charleslmunger 9y ago
    I believe the guarantees around final fields were only added in java 7 - possibly the author is using something older?
    - aardvark179 9y ago
      Nope, the current JMM dates from 2004.
    - xxs 9y ago
      Since 1.5 (and backported to 1.4)
- pvg 9y ago
  That's a confusing and glossed over bit in the article. After all the talk of BigDecimal, the test produces an NPE in String but then the conclusion is the problem is in BigDecimal. The String thing should be far more surprising and unexpected than any secondary effect in BigDecimal.
- LgWoodenBadger 9y ago
  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.
  - pvg 9y ago
    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.
    - LgWoodenBadger 9y ago
      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.
      - pvg 9y ago
        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.
        LgWoodenBadger 9y ago
        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.

needusername 9y ago

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

aardvark179 9y ago
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.
- mmastrac 9y ago
  I agree. The Java memory model explicitly stated that final fields will be correctly initialized in any object reference visible from another thread, provided a reference to that object is only stored after the constructor completes.
- johncolanduoni 9y ago
  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.
  - MichaelGG 9y ago
    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).
    - aardvark179 9y ago
      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 thread. The JMM even says: > If a reference to an object is shared with other threads during the initial construction of an object, most of the guarantees for final fields of that object can go kerflooey; this includes cases in which other parts of a program continue to use the original value of this field.
      - MichaelGG 9y ago
        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.
        aardvark179 9y ago
        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.
- needusername 9y ago
  That would be consistent with my understanding of the JMM and unfortunately my prejudices against "community maintained" backends and ports of OpenJDK. I would not be surprised if this surfaces on other places as well, AFAIK the JDK is full of such benign data races: https://www.youtube.com/watch?v=UykhZ36W04I&index=13&list=PLX8CzqL3ArzXJ2EGftrmz4SzS6NRr6p2n https://www.youtube.com/watch?v=UykhZ36W04I&index=13&list=PL...
- charleslmunger 9y ago
  I believe the guarantees around final fields were only added in java 7 - possibly the author is using something older?
  - aardvark179 9y ago
    Nope, the current JMM dates from 2004.
  - xxs 9y ago
    Since 1.5 (and backported to 1.4)
pvg 9y ago
That's a confusing and glossed over bit in the article. After all the talk of BigDecimal, the test produces an NPE in String but then the conclusion is the problem is in BigDecimal. The String thing should be far more surprising and unexpected than any secondary effect in BigDecimal.
LgWoodenBadger 9y ago
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.
- pvg 9y ago
  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.
  - LgWoodenBadger 9y ago
    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.
    - pvg 9y ago
      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.
      - LgWoodenBadger 9y ago
        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.