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

> 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.