CODEWITHSUNDEEP
javamultithreadingspring
Guest
Guest

Reputation: 455

Thread Safe implementation for In-Memory cache

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

Upvotes: 3

Views: 2697

Answers (1)

ares
ares

Reputation: 4413

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.

Upvotes: 4

Related Questions