Hi Mandy, Thank you for the review, please see below
On Dec 2, 2014, at 2:56 PM, Mandy Chung <mandy.ch...@oracle.com> wrote: > On 12/1/14 8:52 AM, Lance Andersen wrote: >> Hi all, >> >> Looking for a review for this change to DriverManager to reduce the >> possibility on a deadlock on <clinit>. that I have been discussing with >> Mandy. >> >> >> The change removes the static initializer block as well as the synchronized >> keyword from registerDriver. >> >> The webrev can be found at >> http://cr.openjdk.java.net/%7Elancea/8060068/webrev.02/ >> > initDriversIfNeeded() is called to ensure that the drivers are registered. This is a one time operation only those specified via the system property of supporting ServiceLoader. A user could invoke Class.forName(<my java.sql.Driver impl>) and cause a driver to be registered at any time > So it may be better to have a getter method to ensure driver classes are > loaded as well as return the registeredDrivers copy-on-write-arraylist. > i.e. you could rename initDriversIfNeeded to getRegisteredDrivers and > replace > for(DriverInfo aDriver : registeredDrivers) { > > with > for(DriverInfo aDriver : getRegisteredDrivers()) I can do that if you think that is a cleaner approach? I am flexible :-) > > line 564-567: this comment should belong to loadInitialDrivers right? I had left it there as both methods kind of flow together, but I moved it per your suggestion. Thank you > Nit line 567 - not align (one more space to the right needed) fixed > > Is driversSync object necessary? I am not 100% sure but I thought having its own monitor would further reduce the contention possibility and given this was only reported in this one case, I was playing it safe :-). > Can you make loadInitialDrivers be > a synchronized method and move line 575-581 except 578 to the > loadInitialDrivers method (driversInitialized is volatile which is > good). That is doable, I just thought keeping the DCL code together makes it easier to read and decided to have its own monitor. If you feel we should go this way, I will, just let me know Best, Lance > > Mandy Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
