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). 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. 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.ander...@oracle.com>: > 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 > > >
