Bernd, Thank you for your input
On Dec 2, 2014, at 4:49 PM, Bernd Eckenfels <[email protected]> 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 <[email protected]>: > >> Hi Mandy, >> >> Thank you for the review, please see below >> >> On Dec 2, 2014, at 2:56 PM, Mandy Chung <[email protected]> >> 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 >> [email protected] >> >> >> > Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 [email protected]
