9:09 a.m.
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