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?

[A MongoClient is already a pool of connections](http://docs.mongodb.org/ecosystem/drivers/java-concurrency/), why do you want/need to add an extra layer? — assylias, Mar 13 '14 at 10:13

Eyal Schneider · Accepted Answer · 2014-03-13T11:23:04.813

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);
    }    
  }
}

score 0 · Answer 2 · answered Mar 13 '14 at 10:18

0

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.

answered Mar 13 '14 at 10:18

Marko Topolnik

195,646
29
319
436

Ah yes, that was one of my greatest frustrations with respect to what I could do with a Clojure one-liner, but could only achieve in Java with a massive and clunky block of code copypasted for each usage. Naturally, they jumped at the opportunity to leverage the lambda here. – Marko Topolnik Mar 13 '14 at 15:01

Side Effects of Multiton Pattern Usage

2 Answers2