Locking/Singleton in JCAUtil

[Sean Mullan]

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