Instance-controlled classes and multithreading

Question

In Effective Java Chapter 2, Item 1 Bloch suggests to consider static factory methods instead of constructors to initalize an object. On of the benefits he mentions is that this pattern allows classes to return the same object from repeated invocations:

The ability of static factory methods to return the same object from repeated invocations allows classes to maintain strict control over what instances exist at any time. Classes that do this are said to be instance-controlled. There are several reasons to write instance-controlled classes. Instance control allows a class to guarantee that it is a singleton (Item 3) or noninstantiable (Item 4). Also, it allows an immutable class (Item 15) to make the guarantee that no two equal instances exist: a.equals(b) if and only if a==b.

How would this pattern work in a multi threaded environment? For example, I have an immutable class that should be instance-controlled because only one entity with the given ID can exist at a time:

public class Entity {

    private final UUID entityId;

    private static final Map<UUID, Entity> entities = new HashMap<UUID, Entity>();

    private Entity(UUID entityId) {
        this.entityId = entityId;
    }

    public static Entity newInstance(UUID entityId) {
        Entity entity = entities.get(entityId);
        if (entity == null) {
            entity = new Entity(entityId);
            entities.put(entityId, entity);
        }
        return entity;
    }

}

What would happen if I call newInstance() from separated threads? How can I make this class thread-safe?

Answer 1

Make newInstance synchronized that is

public static synchronized Entity newInstance(UUID entityId){
     ...
}

so that one a thread enters the new instance method, no other thread can invoke this method unless the first thread finishes. Basically what happens is the first thread gets a lock for the entire class. For time that the first thread holds the lock for the class, no other thread can enter a synchronized static method for that class.

Answer 2

If you run this code it could lead to unpredictable results, as two threads can at the same time invoke newInstance method, both would see the entity field as null and both would create new Entity. In that case those two threads would have different instances of this class.

You should have a static private field Entity entity in your class instead of getting it from the map. That's why you should use synchronization. You could synchronize the whole method like that:

public synchronized static Entity newInstance(UUID entityId)

As an alternative you could use Double Check Locking which is better, but has to be done carefully - take a look at comments below.

As to the thread safety of this class there's another matter - the map you are using. It makes the class Mutable because the state of the Entity object changes when the map is changed. Final is not sufficient in that case. You should store the map in some other class like EntityManager.

I think your entity should be simple and should not be interested in the question 'am I unique' - it should be someone elses duty. So thats why I would suggest that Entity looked like this:

public class Entity {

    private final UUID entityId;

    public Entity(UUID entityId) {
        this.entityId = entityId;
    }

    public UUID getEntityId() {
        return entityId;
    }
}

Now it's Immutable and will stay that way because its field is final and Immutable. If you want to add some fields make sure those are Immutable as well.

As for storing I would suggest some holder class:

import java.util.Map;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;

public class EntityHolder {
    private static Map<UUID, Entity> entities;

    private static volatile EntityHolder singleton;

    public EntityHolder() {
        entities = new ConcurrentHashMap<UUID, Entity>();
    }

    public Entity getEntity(final UUID id) {
        return entities.get(id);
    }

    public boolean addEntity(final UUID id, final Entity entity) {
        synchronized (entities) {
            if (entities.containsKey(id)) {
                return false;
            } else {
                entities.put(id, entity);
                return true;
            }
        }
    }

    public void removeEntity(final UUID id) {
        entities.remove(id);
    }

    public static EntityHolder getInstance() {
        if (singleton == null) {
            synchronized (EntityHolder.class) {
                if (singleton == null) {
                    singleton = new EntityHolder(); 
                }
            }
        }
        return singleton;
    }
}

This way you have it separated yet accessible from all other classes. And as for creating I would use a creator(factory) like this:

import java.util.UUID;

public class EntityCreator {

public static void createEntity(final UUID id) {
    boolean entityAdded = EntityHolder.getInstance().addEntity(id, new Entity(id));
    if (entityAdded) {
        System.out.println("Entity added.");
    } else {
        System.out.println("Entity already exists.");
    }
}
}