From e0a63f32286357c441a3aa91d8473e0873671b16 Mon Sep 17 00:00:00 2001
From: Endi Sukma Dewata <edewata@redhat.com>
Date: Tue, 25 Sep 2012 21:40:04 -0500
Subject: [PATCH] Fixed synchronization problem in CertificateRepository.

Some synchronized methods in CertificateRepository may block
modifyCeritifcateRecord() too long, so they have been moved
back into CRLIssuingPoint and CertStatusUpdateThread.

Ticket #313
---
 base/ca/src/com/netscape/ca/CRLIssuingPoint.java   |  26 +++++-
 .../certsrv/dbs/certdb/ICertificateRepository.java |  12 ---
 .../cmscore/dbs/CertificateRepository.java         | 104 +++++++++------------
 3 files changed, 70 insertions(+), 72 deletions(-)

diff --git a/base/ca/src/com/netscape/ca/CRLIssuingPoint.java b/base/ca/src/com/netscape/ca/CRLIssuingPoint.java
index c29bc326d7dd6cbea5180e1fae0e370708ac65e3..db30d3988fcba5c0774543a86156c44a958a4cba 100644
--- a/base/ca/src/com/netscape/ca/CRLIssuingPoint.java
+++ b/base/ca/src/com/netscape/ca/CRLIssuingPoint.java
@@ -62,6 +62,8 @@ import com.netscape.certsrv.common.Constants;
 import com.netscape.certsrv.common.NameValuePairs;
 import com.netscape.certsrv.dbs.EDBNotAvailException;
 import com.netscape.certsrv.dbs.IElementProcessor;
+import com.netscape.certsrv.dbs.certdb.ICertRecord;
+import com.netscape.certsrv.dbs.certdb.ICertRecordList;
 import com.netscape.certsrv.dbs.certdb.ICertificateRepository;
 import com.netscape.certsrv.dbs.certdb.IRevocationInfo;
 import com.netscape.certsrv.dbs.crldb.ICRLIssuingPointRecord;
@@ -1868,7 +1870,29 @@ public class CRLIssuingPoint implements ICRLIssuingPoint, Runnable {
      */
     public void processRevokedCerts(IElementProcessor p)
             throws EBaseException {
-        mCertRepository.processRevokedCerts(p, getFilter(), mPageSize);
+        CertRecProcessor cp = (CertRecProcessor) p;
+        String filter = getFilter();
+
+        // NOTE: dangerous cast.
+        // correct way would be to modify interface and add
+        // accessor but we don't want to touch the interface
+        CertificateRepository cr = (CertificateRepository) mCertRepository;
+
+        synchronized (cr.certStatusUpdateTask) {
+            CMS.debug("Starting processRevokedCerts (entered lock)");
+            ICertRecordList list = mCertRepository.findCertRecordsInList(
+                    filter,
+                    new String[] {
+                            ICertRecord.ATTR_ID, ICertRecord.ATTR_REVO_INFO, "objectclass"
+                    },
+                    "serialno",
+                    mPageSize);
+
+            int totalSize = list.getSize();
+
+            list.processCertRecords(0, totalSize - 1, cp);
+            CMS.debug("processRevokedCerts done");
+        }
     }
 
     /**
diff --git a/base/common/src/com/netscape/certsrv/dbs/certdb/ICertificateRepository.java b/base/common/src/com/netscape/certsrv/dbs/certdb/ICertificateRepository.java
index d442802377dfcc3f5a1d3639a54d8a0035ec214a..d54cfb3536bab5e320b84854b73493444d686c68 100644
--- a/base/common/src/com/netscape/certsrv/dbs/certdb/ICertificateRepository.java
+++ b/base/common/src/com/netscape/certsrv/dbs/certdb/ICertificateRepository.java
@@ -28,7 +28,6 @@ import netscape.security.x509.X509CertImpl;
 
 import com.netscape.certsrv.base.EBaseException;
 import com.netscape.certsrv.base.MetaInfo;
-import com.netscape.certsrv.dbs.IElementProcessor;
 import com.netscape.certsrv.dbs.ModificationSet;
 import com.netscape.certsrv.dbs.repository.IRepository;
 import com.netscape.cmscore.dbs.CertificateRepository.RenewableCertificateCollection;
@@ -513,16 +512,5 @@ public interface ICertificateRepository extends IRepository {
      */
     public void removeCertRecords(BigInteger beginS, BigInteger endS) throws EBaseException;
 
-    /**
-     * Builds a list of revoked certificates to put them into CRL.
-     * Calls certificate record processor to get necessary data
-     * from certificate records.
-     * This also regenerates CRL cache.
-     *
-     * @param cp certificate record processor
-     * @exception EBaseException if an error occurred in the database.
-     */
-    public void processRevokedCerts(IElementProcessor cp, String filter, int pageSize) throws EBaseException;
-
     public void shutdown();
 }
diff --git a/base/common/src/com/netscape/cmscore/dbs/CertificateRepository.java b/base/common/src/com/netscape/cmscore/dbs/CertificateRepository.java
index 97bc72ab36952e98198cc43ac11a95a3716b03fd..de9ede52955c2c04f6f34f64f6f69cd3c701b7d1 100644
--- a/base/common/src/com/netscape/cmscore/dbs/CertificateRepository.java
+++ b/base/common/src/com/netscape/cmscore/dbs/CertificateRepository.java
@@ -49,7 +49,6 @@ import com.netscape.certsrv.dbs.IDBSSession;
 import com.netscape.certsrv.dbs.IDBSearchResults;
 import com.netscape.certsrv.dbs.IDBSubsystem;
 import com.netscape.certsrv.dbs.IDBVirtualList;
-import com.netscape.certsrv.dbs.IElementProcessor;
 import com.netscape.certsrv.dbs.Modification;
 import com.netscape.certsrv.dbs.ModificationSet;
 import com.netscape.certsrv.dbs.certdb.ICertRecord;
@@ -86,10 +85,8 @@ public class CertificateRepository extends Repository
     private int mTransitMaxRecords = 1000000;
     private int mTransitRecordPageSize = 200;
 
-    CertStatusUpdateTask certStatusUpdateTask;
-    RetrieveModificationsTask retrieveModificationsTask;
-
-    IRepository requestRepository;
+    public CertStatusUpdateTask certStatusUpdateTask;
+    public RetrieveModificationsTask retrieveModificationsTask;
 
     /**
      * Constructs a certificate repository.
@@ -230,9 +227,6 @@ public class CertificateRepository extends Repository
             boolean listenToCloneModifications) {
 
         CMS.debug("In setCertStatusUpdateInterval " + interval);
-        synchronized (this) {
-            this.requestRepository = requestRepository;
-        }
 
         // stop running tasks
         if (certStatusUpdateTask != null) {
@@ -261,62 +255,31 @@ public class CertificateRepository extends Repository
         }
 
         CMS.debug("In setCertStatusUpdateInterval scheduling cert status update every " + interval + " seconds.");
-        certStatusUpdateTask = new CertStatusUpdateTask(this, interval);
+        certStatusUpdateTask = new CertStatusUpdateTask(this, requestRepository, interval);
         certStatusUpdateTask.start();
     }
 
-    /**
-     * This method blocks when another thread (such as the CRL Update) is running
-     */
-    public synchronized void updateCertStatus() {
+    public void updateCertStatus() throws EBaseException {
 
         CMS.debug("In updateCertStatus()");
 
-        try {
-            CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
-                    CMS.getLogMessage("CMSCORE_DBS_START_VALID_SEARCH"));
-            transitInvalidCertificates();
-            CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
-                    CMS.getLogMessage("CMSCORE_DBS_FINISH_VALID_SEARCH"));
-
-            CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
-                    CMS.getLogMessage("CMSCORE_DBS_START_EXPIRED_SEARCH"));
-            transitValidCertificates();
-            CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
-                    CMS.getLogMessage("CMSCORE_DBS_FINISH_EXPIRED_SEARCH"));
-
-            CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
-                    CMS.getLogMessage("CMSCORE_DBS_START_REVOKED_EXPIRED_SEARCH"));
-            transitRevokedExpiredCertificates();
-            CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
-                    CMS.getLogMessage("CMSCORE_DBS_FINISH_REVOKED_EXPIRED_SEARCH"));
-
-            CMS.debug("Starting cert checkRanges");
-            checkRanges();
-            CMS.debug("cert checkRanges done");
-
-            CMS.debug("Starting request checkRanges");
-            requestRepository.checkRanges();
-            CMS.debug("request checkRanges done");
-
-        } catch (Exception e) {
-            CMS.debug("updateCertStatus done: " + e.toString());
-        }
-    }
-
-    public synchronized void processRevokedCerts(IElementProcessor p, String filter, int pageSize)
-            throws EBaseException {
-
-        CMS.debug("Starting processRevokedCerts (entered lock)");
-        ICertRecordList list = findCertRecordsInList(filter,
-                new String[] { ICertRecord.ATTR_ID, ICertRecord.ATTR_REVO_INFO, "objectclass" },
-                "serialno",
-                pageSize);
-
-        int totalSize = list.getSize();
-
-        list.processCertRecords(0, totalSize - 1, p);
-        CMS.debug("processRevokedCerts done");
+        CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
+                CMS.getLogMessage("CMSCORE_DBS_START_VALID_SEARCH"));
+        transitInvalidCertificates();
+        CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
+                CMS.getLogMessage("CMSCORE_DBS_FINISH_VALID_SEARCH"));
+
+        CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
+                CMS.getLogMessage("CMSCORE_DBS_START_EXPIRED_SEARCH"));
+        transitValidCertificates();
+        CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
+                CMS.getLogMessage("CMSCORE_DBS_FINISH_EXPIRED_SEARCH"));
+
+        CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
+                CMS.getLogMessage("CMSCORE_DBS_START_REVOKED_EXPIRED_SEARCH"));
+        transitRevokedExpiredCertificates();
+        CMS.getLogger().log(ILogger.EV_SYSTEM, ILogger.S_OTHER,
+                CMS.getLogMessage("CMSCORE_DBS_FINISH_REVOKED_EXPIRED_SEARCH"));
     }
 
     /**
@@ -1943,12 +1906,15 @@ public class CertificateRepository extends Repository
 class CertStatusUpdateTask implements Runnable {
 
     CertificateRepository repository;
+    IRepository requestRepository;
+
     int interval;
 
     ScheduledExecutorService executorService;
 
-    public CertStatusUpdateTask(CertificateRepository repository, int interval) {
+    public CertStatusUpdateTask(CertificateRepository repository, IRepository requestRepository, int interval) {
         this.repository = repository;
+        this.requestRepository = requestRepository;
         this.interval = interval;
     }
 
@@ -1963,7 +1929,27 @@ class CertStatusUpdateTask implements Runnable {
     }
 
     public void run() {
+        try {
+            CMS.debug("About to start updateCertStatus");
+            updateCertStatus();
+
+        } catch (EBaseException e) {
+            CMS.debug("updateCertStatus done: " + e.toString());
+        }
+    }
+
+    public synchronized void updateCertStatus() throws EBaseException {
+        CMS.debug("Starting updateCertStatus (entered lock)");
         repository.updateCertStatus();
+        CMS.debug("updateCertStatus done");
+
+        CMS.debug("Starting cert checkRanges");
+        repository.checkRanges();
+        CMS.debug("cert checkRanges done");
+
+        CMS.debug("Starting request checkRanges");
+        requestRepository.checkRanges();
+        CMS.debug("request checkRanges done");
     }
 
     public void stop() {
-- 
1.7.11.4

