RFR 8060068 : Remove the static initializer block from DriverManager

[Lance Andersen]

On Dec 2, 2014, at 4:12 PM, Mandy Chung <mandy.chung at oracle.com> wrote:

> On 12/2/14 12:30 PM, Lance Andersen wrote:
>> 
>>> 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 :-)
> 
> I think so.  It wasn't obvious to me at the beginning when you need to ensure drivers are initialized and whether any place needs to call initDirversIfNeeded.  IMO, having the getter method helps the reader easier to understand and future maintenance.

OK, thank you, done
> 
>>> 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 
>> 
> 
> The lock is to make sure the drivers are loaded once. 
correct
> AFAICT, there is no other method locking DriverManager class except getConnection and I don't see any issue.
Ok
> 
> I forgot to include this comment in my previous reply:
>         /*
>          * When callerCl is null, we should check the application's
>          * (which is invoking this class indirectly)
>          * classloader, so that the JDBC driver class outside rt.jar
>          * can be loaded from here.
>          */
>         ClassLoader callerCL = caller != null ? caller.getClassLoader() : null;
>         synchronized(DriverManager.class) {
>             // synchronize loading of the correct classloader.
>             if (callerCL == null) {
>                 callerCL = Thread.currentThread().getContextClassLoader();
>             }
>         }
> 
> I don't understand why this needs to synchronize in order to get TCCL.
> You are getting the TCCL of the current thread and callerCL is a local
> variable. So I think this can be removed.

I removed this as  I am not sure why that change was made (not by me).

I kicked off a build and will run the  jck tests after it completes

The revised webrev is here http://cr.openjdk.java.net/~lancea/8060068/webrev.03/

Note, I also tweaked the  doPriviliged block for the  JDBC property

Best,
Lance
> Mandy



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com