david david - 12 days ago 5
Java Question

Is my getStatement method thread safe?

I have a below Singleton class where in my

getStatement
method, I populate a CHM by doing if check.

public class CacheHolder {
private static final Map<String, PreparedStatement> holder = new ConcurrentHashMap<>();

private static class Holder {
private static final CacheHolder INSTANCE = new CacheHolder();
}

public static CacheHolder getInstance() {
return Holder.INSTANCE;
}

private CacheHolder() {}

public BoundStatement getStatement(String cql) {
Session session = TestUtils.getInstance().getSession();
PreparedStatement ps = holder.get(cql);
if (ps == null) {
ps = session.prepare(cql);
holder.put(cql, ps);
}
return ps.bind();
}
}


Is my
getStatement
method thread safe?

Answer

The answer provided by @javaguy is right, but just a small optimization to ensure synchronized block doesn't get executed for every thread when its not needed.

public static BoundStatement getStatement(String cql) {
    PreparedStatement ps = null;
    Session session = null;
    try {
      session = TestUtils.getInstance().getSession();
      PreparedStatement ps = holder.get(cql);
      if(ps == null) { // If PS is already present in cache, then we don't have to synchronize and make threads wait.
        synchronized {
          ps = holder.get(cql);
          if (ps == null) {
            ps = session.prepare(cql);
            holder.put(cql, ps);
          }
        }
      }
    } finally {
        //release the resources
    }
    return ps.bind();
  }

You can also use Guava Cache or if you want a Map then Guava MapMaker.

Using Guava Cache:

LoadingCache<String, PreparedStatement> cache = CacheBuilder.newBuilder()
       .maximumSize(1000)
       .expireAfterWrite(10, TimeUnit.MINUTES)
       .build(
           new CacheLoader<String, PreparedStatement>() {
             public PreparedStatement load(String cql) throws Exception {
               return createPreparedStatement(cql);
             }
           });

Using Map Maker :

ConcurrentMap<String, PreparedStatement> cache = new MapMaker()
       .concurrencyLevel(32)
       .weakValues()
       .makeComputingMap(
           new Function<String, PreparedStatement>() {
             public PreparedStatement apply(String cql) {
               return createPreparedStatement(cql);
             }
           });

Also, I would suggest not to cache PreparedStatement's as these resources need to released AFAIK.