Thread Safe implementation for In-Memory cache

Question

import java.io.IOException;
import java.lang.ref.SoftReference;
import java.net.URI;
import java.security.cert.CRLException;
import java.security.cert.CertificateException;
import java.security.cert.X509CRL;
import java.security.cert.X509Certificate;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;

import javax.naming.NamingException;

import org.joda.time.DateTime;
import org.kp.oppr.esb.logger.Logger;
import org.springframework.beans.factory.annotation.Autowired;

public class CachedCrlRepository {

    private static final Logger LOGGER = new Logger("CachedCrlRepository");

    private final Map<URI, SoftReference<X509CRL>> crlCache = Collections
            .synchronizedMap(new HashMap<URI, SoftReference<X509CRL>>());;

    private static int DEFAULT_CACHE_AGING_HOURS;

    @Autowired
    private DgtlSgntrValidator validator;

    @Autowired
    private  CrlRepository crlRepository;

    public X509CRL findCrl(URI crlUri, X509Certificate issuerCertificate,
            Date validationDate) throws DigitalValdiationException,
            CertificateException, CRLException, IOException, NamingException {
        SoftReference<X509CRL> crlRef = this.crlCache.get(crlUri);
        if (null == crlRef) {
            LOGGER.info("Key CRL URI : " + crlUri +  "  not found in the cache " );
            return refreshCrl(crlUri, issuerCertificate, validationDate);
        }
        X509CRL crl = crlRef.get();
        if (null == crl) {
            LOGGER.info("CRL Entry garbage collected: " + crlUri);
            return refreshCrl(crlUri, issuerCertificate, validationDate);
        }
        if (validationDate.after(crl.getNextUpdate())) {
            LOGGER.info("CRL URI  no longer valid: " + crlUri);
            LOGGER.info("CRL validation date: " + validationDate + " is after CRL next update date: " + crl.getNextUpdate());
            return refreshCrl(crlUri, issuerCertificate, validationDate);
        }

        Date thisUpdate = crl.getThisUpdate();
        LOGGER.info("This update " + thisUpdate);

        /*
         * The PKI the nextUpdate CRL extension indicates 7 days. The
         * actual CRL refresh rate is every 3 hours. So it's a bit dangerous to
         * only base the CRL cache refresh strategy on the nextUpdate field as
         * indicated by the CRL.
         */

        DateTime cacheMaturityDateTime = new DateTime(thisUpdate)
                .plusHours(DEFAULT_CACHE_AGING_HOURS);
        LOGGER.info("Cache maturity Date Time " + cacheMaturityDateTime);
        if (validationDate.after(cacheMaturityDateTime.toDate())) {
            LOGGER.info("Validation date: "  + validationDate + " is after cache maturity date: " + cacheMaturityDateTime.toDate());
            return refreshCrl(crlUri, issuerCertificate, validationDate);
        }
        LOGGER.info("using cached CRL: " + crlUri);
        return crl;
    }

    public static int getDEFAULT_CACHE_AGING_HOURS() {
        return DEFAULT_CACHE_AGING_HOURS;
    }

    public static void setDEFAULT_CACHE_AGING_HOURS(int dEFAULT_CACHE_AGING_HOURS) {
        DEFAULT_CACHE_AGING_HOURS = dEFAULT_CACHE_AGING_HOURS;
    }

    private X509CRL refreshCrl(URI crlUri, X509Certificate issuerCertificate,
            Date validationDate) throws DigitalValdiationException,
            CertificateException, CRLException, IOException, NamingException {
        X509CRL crl = crlRepository.downloadCRL(crlUri.toString());
        this.crlCache.put(crlUri, new SoftReference<X509CRL>(crl));
        return crl;
    }




}

I have this class CachedCrlrepository that stores CRL list from particular provider. I want to know if my implementation is thread safe or I am missing something over here. The cache is for a web service, so it is multi-threaded.

My doubt in for this particular method

private X509CRL refreshCrl(URI crlUri, X509Certificate issuerCertificate,
                Date validationDate) throws DigitalValdiationException,
                CertificateException, CRLException, IOException, NamingException {
            X509CRL crl = crlRepository.downloadCRL(crlUri.toString());
            this.crlCache.put(crlUri, new SoftReference<X509CRL>(crl));
            return crl;
        }

I think this particular line need to be synchronized

this.crlCache.put(crlUri, new SoftReference<X509CRL>(crl));

 synchronized(this)
{
this.crlCache.put(crlUri, new SoftReference<X509CRL>(crl));

}

Another issue which I see is that after a GC is run the cache still have that entry in the memory. It never execute these line of code

if (null == crl) {
            LOGGER.info("CRL Entry garbage collected: " + crlUri);
            return refreshCrl(crlUri, issuerCertificate, validationDate);
        }

Am actually confused, do you want it to be thread-safe or not — Saurabh Jhunjhunwala
– Saurabh Jhunjhunwala, Commented May 24, 2015 at 4:19
Re. put(): no, the call to Collections.synchronizedMap() does that for you. I don't like the use of SoftReference though, it's not really good practice. — markspace
– markspace, Commented May 24, 2015 at 4:48
I don't believe that it is possible to answer this question in an authoritative and correct way. The answer to the question "is this thread safe" necessarily must depend on the thread safety policy that has been established for a class or classes and whether the use of instances of those classes violates that policy. Since you don't articulate the policies, there's no way to know. At the end of the day, thread safety is only about protecting mutable state ... and there are a variety of ways to that goal (and a variety of ways to make missteps). — scottb
– scottb, Commented May 24, 2015 at 5:11

ares · Accepted Answer · 2015-05-24 05:24:48Z

4

Generally you should not use a synchronized Map in cases where you are expecting heavy traffic and high concurrent access on your object which in this case is crlCache. For each and every read or write threads will wait behind another and in heavy load, your thread count will go high and eventually your server will crash. You can look into ConcurrentHashMap. which is designed to work efficiently in such scenarios.

Your second point:

synchronized(this)
{
this.crlCache.put(crlUri, new SoftReference<X509CRL>(crl));

}

is not at all required with current code as put method is already synchronized.

For minimal changes replace

private final Map<URI, SoftReference<X509CRL>> crlCache = Collections
            .synchronizedMap(new HashMap<URI, SoftReference<X509CRL>>());;

with

private final ConcurrentHashMap<URI, SoftReference<X509CRL>> crlCache = new ConcurrentHashMap<URI, SoftReference<X509CRL>>();

Finally, using SoftReference is good but there are better options. Guava from google is a very robust and efficient cache builder.

edited May 24, 2015 at 5:24

answered May 24, 2015 at 4:47

ares

4,4236 gold badges37 silver badges65 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

markspace Over a year ago

Well, not "must" but I agree ConcurrentHashMap is probably a better choice if there's any chance of high demand/threads.

scottb Over a year ago

"First of all don't use a synchronized Map for a multi threaded environment as they cause terrible performance issues." Of course, this is bad advice. Synchronized collections are perfectly acceptable solutions for any situation in which contention will be low. Serialization of object state accesses is expected to harm performance only in those situations in which concurrent access is high thereby producing high contention. Moreover, the requirement for atomicity in situations for which concurrent access is not available makes synchronization a simple and, often, clean first-pass solution.

ares Over a year ago

@scottb: Thanks for the advice, made edits accordingly to put a more modest statement.

Collectives™ on Stack Overflow

Thread Safe implementation for In-Memory cache

1 Answer 1

3 Comments

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

3 Comments

Related