Understanding Thread Safety

Question

I'm trying to understand whether this is thread safe. I believe it is, but someone recently brought into question the thread safety of this method.

Let's say I have some factory FactoryA that give us a class which implements the following interface:

public abstract class MyFactory {
    private ObjectA object;
    public void init(ObjectA object){
        _object = object;
    }
}

So, we have something like

public class FactoryA extends MyFactory {
    static final createFactoryClass(String name) {
        // ...ignorning safety checks for brevity
        return MY_MAP.get(name).newInstance();
    }
}

Now, I have some method in another class that takes the factory and gives back a map of possible classes:

public class OtherClass {
    private static FactoryA _factory = new FactoryA();
    private static final Map<String, SomeClass> MY_MAP = new ImmutableMap.Builder<String, MyClass>()
    .put("foo", _factory.createFactoryClass("foo"));

    private SomeObject myMethod(ObjectA objectA, SomeObject someObject) {
        MY_MAP.get(someObject.getString()).init(objectA);
    }
}

The question is whether the init method is thread safe. The map is initialized only once, so even though it's stored in an immutable structure, if two threads call it with different ObjectA, is it possible for the wrong class to use the wrong ObjectA?

Can I just fix this possible problem by doing the following?

private static synchronized myMethod(...) {}

Answer 1

Local variables are always thread-safe, unless the variable is a reference to an object which is shared, i.e. like in the following example:

static void setValue(Example obj, int val) {
    obj.val = val;
}

Example sharedObj = ...;

new Thread(() -> { setValue(sharedObj, 1); }).start();
new Thread(() -> { setValue(sharedObj, 2); }).start();

In that example, though, it's not the use of the reference to sharedObj per se which is unsafe, it's the fact that we used that reference to change the state of sharedObj.val concurrently.

If instead we had two threads which have references to different objects:

Thread threadA = new Thread(() -> {
    Example objA = ...;
    setValue(objA, 1);
});
Thread threadB = new Thread(() -> {
    Example objB = ...;
    setValue(objB, 2);
});
threadA.start();
threadB.start();

The JVM won't get confused and pass e.g. threadA's object to threadB's invocation of setValue, or vice-versa. That's kind of what it sounds like you are asking, and that won't happen. Every thread has its own call stack and each thread's invocation of setValue opens a new stack frame on its own thread. In other words, threadA calls setValue which creates a stack frame on threadA with its own local variables, and threadB calls setValue which creates a stack frame on threadB with its own local variables.

There is a separate concern which is that the changes your init method makes may not be seen by other threads. For example, say you have a Thread thread1 whose job it is to initialize objects by passing them to init, and then pass those objects to some other Thread thread2. Will thread2 see the changes which init made to the objects on thread1? The answer is no, without some sort of memory synchronization, thread2 may not see the changes which were made during thread1's call to init.

Making myMethod synchronized probably doesn't solve that problem, though. Instead, thread1 and thread2 need some way to communicate, like a shared monitor object or lock.

There's a third issue which I guess you are hinting at, which is about the initialization of the map. If the map is built only during a static initializer, then it's thread-safe, because class initialization is performed under synchronization and:

An implementation may optimize this procedure by eliding the lock acquisition [...] when it can determine that the initialization of the class has already completed, provided that, in terms of the memory model, all happens-before orderings that would exist if the lock were acquired, still exist when the optimization is performed.

In other words, static initialization is guaranteed to be seen by other threads, even if the JVM decides that it's been awhile since a particular class has been initialized and decides to read the field without trying to acquire the initialization lock.

Answer 2

I would suggest different naming for the classes/methods, as currently it is difficult to understand what is going on. Also I would make the factories into singletons.

import java.util.HashMap;
import java.util.Map;


public class FactoryStore {

private static final Map<String, AbstractFactory> FACTORIES = new HashMap<>();

static {
    FACTORIES.put("a", FactoryA.getInstance());
    FACTORIES.put("b", FactoryB.getInstance());
}

public static Object createObject(String factoryName, Object parameters) {
    return FACTORIES.get(factoryName).createNewObject(parameters);
}
}



 abstract class AbstractFactory {

Object createNewObject(Object parameters) {
    // not thread-safe stuff
    return new Object();
}


 }



class FactoryA extends AbstractFactory {

private static final FactoryA instance = new FactoryA();

private FactoryA() {
    // thread safe stuff
}

public static FactoryA getInstance() {
    return instance;
}


 }


 class FactoryB extends AbstractFactory {

private static final FactoryB instance = new FactoryB();

 private FactoryB() {
    // thread safe stuff
}

public static FactoryB getInstance() {
    return instance;
}

@Override
synchronized Object createNewObject(Object obj) {
     // can override object creation; this is thread-safe thanks to keyword
    return new Object();
}

 }