Side Effects of Multiton Pattern Usage

Question

I have to need your advice, code-review or improvement about my multiton pattern implementation. I want to multi-connection support for mongodb server.

public class MongoDatabaseFactory {
    private static volatile Map<String, MongoDatabase> connections = new ConcurrentHashMap<String, MongoDatabase>();

    public static MongoDatabase getDatabase(Databases database) throws MongoException {
        if (null == database) throw new MongoException("Database not found");
        if (null == database.name() || database.name().isEmpty()) throw new MongoException("Database not found");

        if (!connections.containsKey(database.name()) || null == connections.get(database.name())) {
            synchronized (database) {
                if (!connections.containsKey(database.name()) || null == connections.get(database.name())) {
                    connectDB(database);
                }
            }
        }

        if (!connections.get(database.name()).isAuthenticated()) {
            synchronized (database) {
                if (!connections.get(database.name()).isAuthenticated()) {
                    connectDB(database);
                }
            }
        }

        return connections.get(database.name());
    }
}

What is best practice for multiton pattern?

Answer 1

As Marko Topolnik says, your current solution isn't thread safe.

I took this as a small exercise, and wrote the following generic thread-safe Multition pattern. Is it designed to perform well with many threads, and it's suitable in cases where the value object creation is expensive. Note that I'm not sure there isn't a simpler solution in your specific case, however.

import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;


public class ThreadSafeMultition <K, V> {
  private final ConcurrentHashMap<K, FutureTask<V>> map = new ConcurrentHashMap<K, FutureTask<V>>();
  private ValueFactory<K, V> factory;

  public ThreadSafeMultition(ValueFactory<K, V> factory) {
    this.factory = factory;
  }

  public V get(K key) throws InterruptedException, ExecutionException {
    FutureTask<V> f = map.get(key);
    if (f == null) {
      f = new FutureTask<V>(new FactoryCall(key));
      FutureTask<V> existing = map.putIfAbsent(key, f);
      if (existing != null)
        f = existing;
      else // Item added successfully. Now that exclusiveness is guaranteed, start value creation.
        f.run();
    } 

    return f.get();
  }

  public static interface ValueFactory<K, V> {
    public V create(K key) throws Exception;
  }

  private class FactoryCall implements Callable<V> {
    private K key;

    public FactoryCall(K key) {
      this.key = key;
    }

    @Override
    public V call() throws Exception {
      return factory.create(key);
    }    
  }
}

Answer 2

This line is not thread-safe:

if (!connections.containsKey(database.name()) || null == connections.get(database.name()))

You'll have a data race on the hash map here because you are not protecting map access with a lock. Probably the best solution would be to move this into the synchronized block. You shouldn't worry about the performance here, at least not without a firm proof.