Multi-threaded Java WTF

powerlord

So, it was about 10 minutes before I left work and I had nothing to work on at the moment, so I decided to view some questions on StackOverflow.

I ran into this question:

Java Memory Visibility In Constructors

For the following simplified class: public class MutableInteger { private int value; public MutableInteger(int initial) { synchronized(this) { // is this necessary for memory visib...

My own answer was going to be "No, values in the constructor are set before you can use them."

However, it turns out that, according to the Java spec, that's wrong. In a multi-threaded environment, it's possible to see the value of a non-final (similar to readonly in C#) field before the constructor sets the value on it unless you explicitly manage access to it.

Seriously, why the fuck would you allow an object to be accessed before it's finished being constructed?

I find this so WTFy that I stayed 5 minutes past the time I'm supposed to leave work just to post this!

powerlord

@powerlord said:

Seriously, why the fuck would you allow an object to be accessed before it's finished being constructed?

On a side note, that doesn't mean the implementation works that way, only that it isn't required by the language specification.

Buddy

The rule is that if you're accessing the same object in multiple threads without synchronization, all bets are off. So synchronized blocks inside the ctor are unnecessary, because there's no way another thread could get a reference to a partially constructed object except by breaking that rule, either on the producing thread or the consuming.

Buddy

The "Double-Checked Locking is Broken" Declaration

dkf

@powerlord said:

Seriously, why the fuck would you allow an object to be accessed before it's finished being constructed?

I had to read the code in the question and the JLS a few times to grok this, but it's not what you think. It's not that the memory is accessed before the constructor finishes, but rather that there's no guarantee of a memory barrier between when the constructor finishes executing and when code outside gets a handle to the object. This in turn means that there's no guarantee that another thread that is given the object in an unsafe way will see that all the assignments that happened in the constructor have actually happened yet; the writes (which have happened) might not yet have been synchronized to the copy of memory that the other thread has.

Given that almost all objects are manipulated in their creation thread before being passed somewhere else (if ever) it's actually good that there's no guarantee of a memory barrier, as that would force everyone to always pay for the possibility that someone might do something really stupid.

cartman82

@powerlord said:

I find this so WTFy that I stayed 5 minutes past the time I'm supposed to leave work just to post this!

We appreciate your sacrifice.

If you're using threads directly in this day and age, you're doing it wrong.

Greybeard

My advice to developers: "There are few developers who understand concurrency. Get your code reviewed by one of them."

powerlord

To be fair, almost all my Java stuff is web development. If I really need a background task to do something, I spin it off from an ExecutorService and chuck the returned Future object into the session then use AJAX calls to see if it's completed yet.

Scarlet_Manuka

@Greybeard said:

My advice to developers: "There are few developers who understand concurrency. Get your code reviewed by ~~one of them~~several of them at the same time."

quijibo

I understand that in theory the original topic with constructors not being synchronized may be an issue because the JVM doesn't guarantee anything. But in practice on x86 architectures I don't think this is an issue at all because of cache coherency guarantees.

I think a good article that explains this is here: http://mechanical-sympathy.blogspot.ca/2013/02/cpu-cache-flushing-fallacy.html

Otherwise, I would guess that any multi-threaded code that uses common library classes like Vector (with its non-final elementData) would be crashing all over the place and everyone should know about this problem by now.

Maybe this kind of thing is an issue on ARM (Android phones), but then again, to keep everyone sane JVMs might have synchronization even if it's not in the spec. No one says that the JVM can't do extra work.

Any way, this all seems like much ado about nothing unless you're running code on a non-x86 CPU... and even then...

dkf

@quijibo said:

Otherwise, I would guess that any multi-threaded code that uses common library classes like Vector (with its non-final elementData) would be crashing all over the place and everyone should know about this problem by now.

It's not usually an issue because as long as any memory barrier happens, the consistency is guaranteed. For this sort of thing to be a problem, you need to be doing something unwise elsewhere such as giving the object handle to another thread via an unguarded shared variable. At that point, you've already Done It Wrong; you're just gambling with what shows you the consequences of that.

Threads are difficult to use well. Fortunately, there are good patterns to use, and classes that do the tricky stuff for you.

Greybeard

@dkf said:

classes that do the tricky stuff for you

Don't always help:

public class VertexStore {
    private final ConcurrentMap<String, List<String>> vertices = new ConcurrentHashMap<>();

    public boolean put(String from, String to) {
        List<String> myVertex = vertices.get(from);
        if (myVertex == null) {
            myVertex = new ArrayList<>();
            vertices.put(from, myVertex);
        }
        return myVertex.add(to);
    }

    public List<String> get(String id) {
        List<String> myVertices = vertices.get(id);
        return myVertices;
    }
}

dkf

@Greybeard said:

Don't always help:

And bad code proves what exactly?

anonymous234

But give them a copy of your code, not a reference. Otherwise you'll have to wait until they all finish before you can change anything.

tar

@dkf said:

And bad code proves what exactly?

That you should feel bad?

Not (necessarily) you you...

Greybeard

@dkf said:

And bad code proves what exactly?

Classes that "do the tricky stuff for you" don't necessarily help mediocre developers write thread safe code. The classes do not confer magic thread safe pixie dust on all code using them.

A developer who cannot understand and apply the visibility rules is going to write bad code. It is my experience that a large proportion of developers writing concurrent code lack such understanding.

dkf

@Greybeard said:

magic thread safe pixie dust

That's the stuff that bad developers snort a line of before believing that they can write thread-enabled code? :D