On 05/22/2014 08:34 PM, Bernd Eckenfels wrote:
Hello,by browsing the source code I run across the JCAUtil class. It is (among other stuff) responsible for providing a SecureRandom singleton. The code looks a bit strange. First of all, it defines a LOCK object, but instead of using an unreachable instancde (which is a common pattern for those kind of LOCK objects) it uses the public class itself: private static final Object LOCK = JCAUtil.class; Typical this would be a problem as I can lock up the class. In this specific case the LOCK is only used in one place, and there it is used for a double checked locking, which is I guess good as it only checks the monitor before any user code can lock it. Nevertheless, I would recommend to change this to a more common pattern (or remove the field and us synchronized(JCAUtil.class) to make it more explicite. Another option would be to get rid of all the volatile/lock/DCL by having a static initialisation. If this is not possible for dependency reasons, I would have expected a comment like "this needs to be lazy because..." With final and without volatile it also looks more predictable: private static final SecureRandom secureRandom = new SecureRandom(); public static SecureRandom getSecureRandom() { return secureRandom; } WDYT?
That would work, but then you lose the performance benefits of the existing code, which only creates the SecureRandom object if the getSecureRandom method is called. I think that using the Initialization-on-demand pattern [1] is a better solution which avoids the locking and is thread-safe. When this code was written, it probably wasn't a well-known pattern.
--Sean [1] http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
