On Dec 2, 2014, at 4:12 PM, Mandy Chung <[email protected]> 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
[email protected]
