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

[Ade Lee]

On Mon, 2012-10-01 at 09:09 -0500, Endi Sukma Dewata wrote:
> 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.
> 
I agree with points 1 and 2.  It makes sense to have
processCertRecords() as a method within CertificateRepository, so that
it can lock its own private thread.  I would be OK with the code in
point 2 above.

I'm not sure I agree that locking using a synchronized method is any
cleaner though.  The patch moves code which does not is not executed by
the maintenance thread into the maintenance thread, purely for the sake
of elucidating synchronization.  The maintenance thread should contain
only the methods that it executes.
   
> 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.
>