RFR 8060068 : Remove the static initializer block from DriverManager

[Lance Andersen]

Bernd,

Thank you for your input

On Dec 2, 2014, at 4:49 PM, Bernd Eckenfels <ecki at zusammenkunft.net> wrote:

> Hello,
> 
> just want to add two somewhat related observations:
> 
> we have a virtual JDBC driver which maps back to an real driver (or to
> an Pool/Datasource which uses a real driver. This is to allow
> JDBC URLs to work in different environments with no config. (Thats is
> not the nices solution, but after we resolved the deadlocks it
> works stable with the legacy code we need to support).

A couple of comments

-  DataSources were never meant to register themselves via DriverManager.  Unfortunately some implementations of DataSource still do. 
-  The changes should reduce the potential  for deadlocks given we no longer use a synchronized method for registeredDriver.  We have been using CopyOnWriteArrayList since JDK 7
-  The removal of the static block with the removal of having registerDriver synchronized will further reduce the probability
> 
> This does have two consequences which are related to this patch:
> 
> a) #getConnection() is used quite often, as it tunnels through to a
> high performing pool (already mentioned as a good reason for DCL).
> 
> b) there have been some deadlock conditions in the past in this area. I
> can try to find the details later on, but it involved class
> loading/registration and the driver's synchronized.
These hopefully are gone with the change, but if you have a simple repro after trying the patch, that would be helpful

Best,
Lance
> 
> So it is a bit risky (assuming we are not the only ones using the SPI
> in this creative way) to play with the locking.
> 
> At the same time, it is also needed. Maybe the more finegrained lock
> and the lock-free happy path help with it. I wonder, is there a
> special procedure to warn in the release notes/compatibility guide
> other than having the patch listed in a changelog? In order to warn
> people switching to 9 (in 5 years or so :).
> 
> Gruss
> Bernd
> 
> 
> Am Tue, 2 Dec
> 2014 15:30:16 -0500 schrieb Lance Andersen <lance.andersen at oracle.com>:
> 
>> Hi Mandy,
>> 
>> Thank you for the review, please see below
>> 
>> On Dec 2, 2014, at 2:56 PM, Mandy Chung <mandy.chung at 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.Andersen at oracle.com
>> 
>> 
>> 
> 



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