Why not just initialize it from a static {} block, then in a test if we want
to change it, we change it?On Tue, May 25, 2010 at 8:54 PM, Ryan Rawson <[email protected]> wrote: > Thanks for that salient comment. Perhaps someone can give us the right > pattern for no lock Singleton initialization. > > On May 25, 2010 8:49 PM, "Paul Cowan" <[email protected]> wrote: > > On 26/05/2010 1:15 PM, Daniel Ploeg wrote: > >>>> this is a lot of synchronization for what ends up being on the fast > path of every query, perhaps there is a non synchronized manner in which we > can accomplish this? the use of volatile might help here > >> > >> thanks Ryan. I think I might use AtomicReference instead - can simplify > even further this way. I'll upload another patch a bit later. > > > > Sorry, no real stake in this review but just a curious third-party: is > > there any need for even the AtomicReference? > > > > Making 'delegate' volatile would completely remove the need for the > > synchronization in getDelegate(), and the lock on injectEdge(). The only > > exception is that without synchronization, the null-check-and-default in > > getDelegate() would be subject to race conditions. Given the desire to > > reduce synchronization, and the fact that delegate is assigned to > > default at construction time, I'd humbly suggest that the "if (delegate > > == null)" be removed from getDelegate(), and instead change injectEdge() > > to reject null values (or call reset() if you want to keep the effective > > existing semantics that injectEdge(null) returns to a > > DefaultEnvironmentEdge). > > > > Then the ReentrantLock can be removed, no synchronization is required, > > and one volatile variable is all that's needed. > > > > (Incidentally, I'm not sure what the ReentrantLock is actually FOR -- > > reference assignment is atomic in Java, so it's not protecting against > > any race conditions; all it could be useful for is a happens-before > > relationship with the get, but the two are using different locks -- the > > synchronized get does not have a happens-before relationship with the > > unlock. Either both should be synchronized, or both use the lock -- as > > it is, the lock buys you nothing. Mind you with a simple volatile then > > the happens-before relationship is correct anyway). > > > > Cheers, > > > > Paul > -- Todd Lipcon Software Engineer, Cloudera
