3
\$\begingroup\$

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) { // 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);
          }
        }
    }
    return ps.bind();
  }
}

I am working with Cassandra and using datastax java driver so I have to re-use prepared statements and that's why I am caching it here. Prepared Statement and BoundStatement.

Is there any better way of making my getStatement method thread safe instead of using synchronized block like that? Any other data structure which might be thread safe for these kind of operations?

I am working with Java 7.

\$\endgroup\$
1
\$\begingroup\$
public BoundStatement getStatement(String cql) {
    Session session = TestUtils.getInstance().getSession();
    PreparedStatement ps = holder.computeIfAbsent(cql, s -> session.prepare(s));
    return ps.bind();
}

According to computeIfAbsent's Javadoc:

The entire method invocation is performed atomically, so the function is applied at most once per key.

I also suggest that you declare holder of type ConcurrentHashMap instead of Map so that it becomes unambiguous for anyone reading getStatement that the synchronisation in computeIfAbsent is guaranteed.

\$\endgroup\$
  • \$\begingroup\$ I am still using Java 7. Can you provide an example how this would work in Java 7? \$\endgroup\$ – david Nov 25 '16 at 14:00
  • \$\begingroup\$ @david Too bad. I can't show you an equivalent since computeIfAbsent was introduced with Java 8. Can you update your post to state you're using Java 7 ? \$\endgroup\$ – Spotted Nov 25 '16 at 14:02
  • \$\begingroup\$ ok just edited.. \$\endgroup\$ – david Nov 25 '16 at 14:31
  • \$\begingroup\$ How about using putIfAbsent method of CHM? \$\endgroup\$ – david Nov 27 '16 at 20:08
  • \$\begingroup\$ @david with putIfAbsent the problem is that the value is not lazy-evaluated, i.e. session.prepare(s) will always be executed even if the value is already present in the map. On the other side, with computeIfAbsent the value is evaluated only if needed. \$\endgroup\$ – Spotted Nov 28 '16 at 7:43
1
\$\begingroup\$

Agree with the previous answer however computeIfAbsent() may block other threads while the computation is being performed

You can also do something like below (Compare-and-swap instead of locking, way more complex than the previous answer, so that answer may be the preferable one) -

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

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

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

private CacheHolder() {
    DUMMY_PS = //Create a dummy PS
}

public BoundStatement getStatement(String cql) {
    Session session = TestUtils.getInstance().getSession();
    PreparedStatement ps, prevValue;

    while(true) {
        ps = holder.get(cql);

        if(ps == null) {
            prevValue = holder.putIfAbsent(cql, DUMMY_PS);
            if (prevValue == null) { // the value before our putIfAbsent() call was null,
                                     // which means the spot was empty and we managed to book it with the DUMMY_VALUE
                                    // we should go ahead and put an entry there
                ps = session.prepare(cql);
                holder.put(cql, ps);
                return ps.bind();
            }
            else if (prevValue == DUMMY_PS) { //Some thread has already put the dummy value for this key, which means it is soon going to put the real thing in there
                continue;
            }
            else { //some thread had put the real thing in before we called putIfAbsent
                return prevValue.bind();
            }
        }
        if (ps == DUMMY_PS) {
            continue; //Some thread is soon going to put the real thing in there
        }
        else {
            return ps.bind();
        }
    }
}

}

\$\endgroup\$
  • \$\begingroup\$ Interesting approach. However, if preparing a statement takes some time, another thread looking for the same statement will use hog CPU time until DUMMY_PS is replaced with the real statement. That's very expensive and can cause a meltdown if a network hickup affects several threads at once. \$\endgroup\$ – Codo Nov 25 '16 at 10:47
  • \$\begingroup\$ True, that is an issue. Maybe other threads should wait on a conditional lock during that time. \$\endgroup\$ – Ashutosh A Nov 25 '16 at 11:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.