Is my getStatement method thread safe?

Question

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 1

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.