looks ok to me, but I's like Andrew to take a look. Ade On Tue, 2012-09-25 at 22:11 -0500, Endi Sukma Dewata wrote:

On 9/28/2012 1:18 PM, Andrew Wnuk wrote: 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

On 10/1/2012 9:44 AM, Ade Lee wrote: New patch attached. As discussed, this patch will now revert the code structure to be similar to the one in the Dogtag 9, meaning the point #1 and #2 above will also be reverted. I hope this is sufficient to address the synchronization issue in ticket #313. Further clean up can be done separately. -- Endi S. Dewata