[Pki-devel] [PATCH] 125 Fixed synchronization problem in CertificateRepository.

[Endi Sukma Dewata]

On 9/28/2012 1:18 PM, Andrew Wnuk wrote:
> On 09/25/2012 08:11 PM, Endi Sukma Dewata wrote:
>> Some synchronized methods in CertificateRepository have been moved
>> into CertStatusUpdateThread to avoid blocking other synchronized
>> methods too long.
>>
>> Ticket #313

> NACK.
>
> processRevokedCerts - seems to be use only by CRLIssuingPoint and it is
> not used by CertificateRepository maintenance thread therefore I so not
> reason to move processRevokedCerts to CertStatusUpdateTask.

Just to clarify the IRC discussion last week, this patch is supposed to 
address the long blocking issue by reverting to Dogtag 9 behavior, 
assuming the Dogtag 9 has the correct behavior.

In Dogtag 9 the processRevokedCerts() exists in CRLIssuingPoint class as 
shown in this simplified code:

   public void processRevokedCerts(IElementProcessor p) {

     // 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.mCertStatusUpdateThread) {
       CMS.debug("Starting processRevokedCerts (entered lock)");

       list = mCertRepository.findCertRecordsInList(...);
       list.processCertRecords(...);

       CMS.debug("processRevokedCerts done");
     }
   }

There are some issues in the above code:

1. In Dogtag 9 the mCertRepository is being downcasted to 
CertificateRepository. As indicated by the comment in the code this is 
dangerous and we are supposed to modify the ICertificateRepository 
interface. In Dogtag 10 this was fixed by adding a new method called 
processRevokedCerts() into the interface so it's no longer necessary to 
downcast mCertRepository. The above code will now look like this:

   public void processRevokedCerts(IElementProcessor p) {
     mCertRepository.processRevokedCerts(...);
   }

2. In Dogtag 9 the code locks the mCertStatusUpdateThread which is an 
attribute of the mCertRepository. This is not a good OO practice because 
it's breaking the encapsulation. We could add a method 
getCertStatusUpdateThread() but it's still not a good solution because 
the thread class is a private class of CertificateRepository. The 
CRLIssuingPoint isn't supposed to see the thread object. This patch 
addresses this issue by moving the above code into 
CertificateRepository's processRevokedCerts(). This way the certificate 
repository can lock its own thread without breaking encapsulation.

   public void processRevokedCerts(IElementProcessor p) {
     synchronized (mCertStatusUpdateThread) {
       CMS.debug("Starting processRevokedCerts (entered lock)");

       list = this.findCertRecordsInList(...);
       list.processCertRecords(...);

       CMS.debug("processRevokedCerts done");
     }
   }

This, however, is not the final code. See the following point.

3. In Dogtag 9 the locking is done using a synchronized block. While it 
works just fine, the code could be implemented with a synchronized 
method which would be cleaner: the object being locked is implicit, and 
here we block the whole method which has a well defined boundary, 
instead of a piece of code in the middle of a method.

With this patch the Dogtag 10 is supposed to have the same behavior as 
Dogtag 9 that they both guarantee mutual exclusion of the following code:

   list = this.findCertRecordsInList(...);
   list.processCertRecords(...);

and the following code:

   _cr.updateCertStatus();
   _cr.checkRanges();
   _rr.checkRanges();

And the execution of either of the above code wouldn't block 
modifyCertificateRecord().

It would be difficult to prove correctness of undeterministic code by 
testing. Testing would prove if there's an error in the code, but no 
error even after extensive testing doesn't mean it's correct. The best 
way to review this patch is I think by analyzing the code, following the 
logic, comparing with previous code, and some basic testing.

-- 
Endi S. Dewata