Derby
  1. Derby
  2. DERBY-2905

Shutting down embedded Derby does not remove all code, the AutoloadDriver is left registered in the DriverManager.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.2.2.0, 10.3.1.4, 10.4.1.3
    • Fix Version/s: 10.8.1.2
    • Component/s: JDBC
    • Labels:
      None
    • Urgency:
      Normal
    • Issue & fix info:
      High Value Fix
    • Bug behavior facts:
      Regression

      Description

      After a shutdown of the embedded driver the AutoloadDriver is not unregistered from DriverManager. However it does not support any future loading of connections so it has no value in remaining registered. Since the DriverManager class will remain forever, this means the Derby code will remain forever in the JVM, even if Derby was loaded by a separate class loader.

      Regression from 10.1 since before the AutoloadedDriver the internal driver did unregister itself from the DriverManager on a shutdown.

      1. ww.java
        0.6 kB
        Rick Hillegas
      2. Repro2905.java
        4 kB
        Lily Wei
      3. releaseNote.html
        5 kB
        Lily Wei
      4. releaseNote.html
        5 kB
        Lily Wei
      5. releaseNote.html
        4 kB
        Knut Anders Hatlen
      6. releaseNote.html
        4 kB
        Rick Hillegas
      7. releaseNote.html
        5 kB
        Rick Hillegas
      8. releaseNote.html
        5 kB
        Rick Hillegas
      9. Mainv1.java
        3 kB
        Ramin Moazeni
      10. Main.java
        3 kB
        Ramin Moazeni
      11. DERBY-2905v3.stat
        0.5 kB
        Ramin Moazeni
      12. DERBY-2905v3.diff
        11 kB
        Ramin Moazeni
      13. DERBY-2905v1.stat
        0.4 kB
        Ramin Moazeni
      14. DERBY-2905v1.diff
        11 kB
        Ramin Moazeni
      15. DERBY-2905v0.stat
        0.1 kB
        Ramin Moazeni
      16. DERBY-2905v0.diff
        2 kB
        Ramin Moazeni
      17. DERBY-2905-2.diff
        11 kB
        Lily Wei
      18. derby-2905-01-aa-fixAutoloadedDriverReload.diff
        1.0 kB
        Rick Hillegas
      19. DERBY-2905_part2_2.diff
        13 kB
        Lily Wei
      20. DERBY-2905_part2_2_3.diff
        7 kB
        Lily Wei
      21. DERBY-2905_part2_2_2.diff
        3 kB
        Lily Wei
      22. DERBY-2905_part2_2_1.diff
        13 kB
        Lily Wei
      23. DERBY-2905_part2_1.diff
        8 kB
        Lily Wei
      24. DERBY-2905_3.diff
        11 kB
        Lily Wei
      25. DERBY-2905_1.diff
        11 kB
        Lily Wei

        Issue Links

          Activity

          Hide
          Kathey Marsden added a comment -

          marking urgent since this is a regression.

          Show
          Kathey Marsden added a comment - marking urgent since this is a regression.
          Hide
          Rick Hillegas added a comment -

          Reverting this issue to normal urgency. This issue occurs in 10.2. I do not believe that this old issue should block the incremental release of other quality improvements.

          Show
          Rick Hillegas added a comment - Reverting this issue to normal urgency. This issue occurs in 10.2. I do not believe that this old issue should block the incremental release of other quality improvements.
          Hide
          Ramin Moazeni added a comment -

          Attaching a reproducible test case with this issue.
          As mentioned in the description, the AutoloadedDriver is not unregistered
          from the DriverManager. The attached test case shows the behavior as
          follows:

            • Check for Autoloaded driver before DB shutdown
          • Testing with org.apache.derby.jdbc.EmbeddedDriver
          • Getting Registered Drivers
            sun.jdbc.odbc.JdbcOdbcDriver
            org.apache.derby.jdbc.AutoloadedDriver
            org.apache.derby.jdbc.ClientDriver
          • Shutting Down Database
            Database shut down normally
            • Check for Autoloaded driver after DB shutdown
          • Getting Registered Drivers
            sun.jdbc.odbc.JdbcOdbcDriver
            org.apache.derby.jdbc.AutoloadedDriver
            org.apache.derby.jdbc.ClientDriver

          in 10.1, the above test case shows Driver30 successfully unregistering after
          database shutdown:

            • Check for Autoloaded driver before DB shutdown
          • Testing with org.apache.derby.jdbc.EmbeddedDriver
          • Getting Registered Drivers
            org.apache.derby.jdbc.Driver30
          • Shutting Down Database
            Database shut down normally
            • Check for Autoloaded driver after DB shutdown
          • Getting Registered Drivers
          Show
          Ramin Moazeni added a comment - Attaching a reproducible test case with this issue. As mentioned in the description, the AutoloadedDriver is not unregistered from the DriverManager. The attached test case shows the behavior as follows: Check for Autoloaded driver before DB shutdown Testing with org.apache.derby.jdbc.EmbeddedDriver Getting Registered Drivers sun.jdbc.odbc.JdbcOdbcDriver org.apache.derby.jdbc.AutoloadedDriver org.apache.derby.jdbc.ClientDriver Shutting Down Database Database shut down normally Check for Autoloaded driver after DB shutdown Getting Registered Drivers sun.jdbc.odbc.JdbcOdbcDriver org.apache.derby.jdbc.AutoloadedDriver org.apache.derby.jdbc.ClientDriver in 10.1, the above test case shows Driver30 successfully unregistering after database shutdown: Check for Autoloaded driver before DB shutdown Testing with org.apache.derby.jdbc.EmbeddedDriver Getting Registered Drivers org.apache.derby.jdbc.Driver30 Shutting Down Database Database shut down normally Check for Autoloaded driver after DB shutdown Getting Registered Drivers
          Hide
          Ramin Moazeni added a comment -

          Attaching an interim patch for this issue.
          The AutoloadedDriver is now unregistering itself from DriverManager
          as follows:

            • Check for Autoloaded driver before DB shutdown
          • Testing with org.apache.derby.jdbc.EmbeddedDriver
          • Getting Registered Drivers
            sun.jdbc.odbc.JdbcOdbcDriver
            org.apache.derby.jdbc.ClientDriver
            org.apache.derby.jdbc.AutoloadedDriver
          • Shutting Down Database
            Database shut down normally
            • Check for Autoloaded driver after DB shutdown
          • Getting Registered Drivers
            sun.jdbc.odbc.JdbcOdbcDriver
            org.apache.derby.jdbc.ClientDriver

          I appreciate your comments/review.

          Thanks
          Ramin

          Show
          Ramin Moazeni added a comment - Attaching an interim patch for this issue. The AutoloadedDriver is now unregistering itself from DriverManager as follows: Check for Autoloaded driver before DB shutdown Testing with org.apache.derby.jdbc.EmbeddedDriver Getting Registered Drivers sun.jdbc.odbc.JdbcOdbcDriver org.apache.derby.jdbc.ClientDriver org.apache.derby.jdbc.AutoloadedDriver Shutting Down Database Database shut down normally Check for Autoloaded driver after DB shutdown Getting Registered Drivers sun.jdbc.odbc.JdbcOdbcDriver org.apache.derby.jdbc.ClientDriver I appreciate your comments/review. Thanks Ramin
          Hide
          Daniel John Debrunner added a comment -

          Patch looks good. Adding a test case to jdbcapi.AutoloadTest would be good.

          Show
          Daniel John Debrunner added a comment - Patch looks good. Adding a test case to jdbcapi.AutoloadTest would be good.
          Hide
          Ramin Moazeni added a comment -

          Thanks dan for your review.
          I am attaching another patch for this issue. I appreciate
          your comments/review.

          Thanks
          Ramin

          Show
          Ramin Moazeni added a comment - Thanks dan for your review. I am attaching another patch for this issue. I appreciate your comments/review. Thanks Ramin
          Hide
          Mamta A. Satoor added a comment -

          Ramin, your patch looks good. I noticed though that for the new entry (JDBC_DRIVER_UNREGISTER_ERROR) in MessageId.java, there was no corresponding error text message entry in org.apache.derby.loc:messages.xml

          Show
          Mamta A. Satoor added a comment - Ramin, your patch looks good. I noticed though that for the new entry (JDBC_DRIVER_UNREGISTER_ERROR) in MessageId.java, there was no corresponding error text message entry in org.apache.derby.loc:messages.xml
          Hide
          Daniel John Debrunner added a comment -

          The first patch was simple, just adding a deregister of the driver.

          The new patch (v1) does a lot more, can you please explain what is going on?

          Show
          Daniel John Debrunner added a comment - The first patch was simple, just adding a deregister of the driver. The new patch (v1) does a lot more, can you please explain what is going on?
          Hide
          Ramin Moazeni added a comment -

          Hello Dan,

          Yes, the v0 patch was simpler however, I noticed that it has some problems.
          Please consider the following scenario:
          1) The DB is shut down and therefore, the Autoloaded driver is
          unregistered.
          2) The user tries to explicitly load the Embedded driver using
          Class.forName(..).newInstance()

          With v0 patch, the user couldn't load the Embedded driver explicitly after
          the DB shutdown and I was getting the "No Suitable Driver" error.

          I think the problem was that the DriverManager.registerDriver() was being
          called in a static block and therefore it was called only once to load the
          AutoloadedDriver. With the changes in v1 patch, the user can explicitly load the Embedded
          driver after the DB shutdown and after the Autoloaded driver is being
          unregistered. The AutoloadedDriver.registerDriverModule(...) takes care of registering the
          driver when the user is explicitly loading the Embedded driver.

          I am attaching a testcase with this issue. If you apply the v0 patch and run the
          testcase, you will see the behavior I am explaining above.

          Thanks
          Ramin

          Show
          Ramin Moazeni added a comment - Hello Dan, Yes, the v0 patch was simpler however, I noticed that it has some problems. Please consider the following scenario: 1) The DB is shut down and therefore, the Autoloaded driver is unregistered. 2) The user tries to explicitly load the Embedded driver using Class.forName(..).newInstance() With v0 patch, the user couldn't load the Embedded driver explicitly after the DB shutdown and I was getting the "No Suitable Driver" error. I think the problem was that the DriverManager.registerDriver() was being called in a static block and therefore it was called only once to load the AutoloadedDriver. With the changes in v1 patch, the user can explicitly load the Embedded driver after the DB shutdown and after the Autoloaded driver is being unregistered. The AutoloadedDriver.registerDriverModule(...) takes care of registering the driver when the user is explicitly loading the Embedded driver. I am attaching a testcase with this issue. If you apply the v0 patch and run the testcase, you will see the behavior I am explaining above. Thanks Ramin
          Hide
          Ramin Moazeni added a comment -

          Thanks Mamta for your review. I am attaching the v3 patch
          addressing your review comments.
          I ran derbyall and suites.All without any errors.

          Thanks
          Ramin

          Show
          Ramin Moazeni added a comment - Thanks Mamta for your review. I am attaching the v3 patch addressing your review comments. I ran derbyall and suites.All without any errors. Thanks Ramin
          Hide
          Daniel John Debrunner added a comment -

          I working on committing this patch.

          Show
          Daniel John Debrunner added a comment - I working on committing this patch.
          Hide
          Daniel John Debrunner added a comment -

          I reworked the test cases in the v3 patch into AutoLoadTest.java and committed them with revision 572822.
          Main changes were:
          don't add an unused method
          split the test cases into two fixtures & reuse some existing code for reloading the embedded driver.
          don't assume JDBC 4

          Show
          Daniel John Debrunner added a comment - I reworked the test cases in the v3 patch into AutoLoadTest.java and committed them with revision 572822. Main changes were: don't add an unused method split the test cases into two fixtures & reuse some existing code for reloading the embedded driver. don't assume JDBC 4
          Hide
          Kathey Marsden added a comment -

          Marking patch available so we hopefully get some comments on this patch.

          Show
          Kathey Marsden added a comment - Marking patch available so we hopefully get some comments on this patch.
          Hide
          Kathey Marsden added a comment -

          On Sept 4, Dan said
          >I working on committing this patch.

          Dan, could you give us an update on your review of this issue? Are there changes needed to the patch?

          Show
          Kathey Marsden added a comment - On Sept 4, Dan said >I working on committing this patch. Dan, could you give us an update on your review of this issue? Are there changes needed to the patch?
          Hide
          Daniel John Debrunner added a comment -

          I think the patch results in code that is brittle, it adds more state about the boot state of the class when it already has a couple of fields trying to manage state.

          In addition its seems the registering of Derby's embedded driver has become overly complex, some due in part to previous changes, also I think the patch means that the autoloaded driver gets re-used on a re-boot which doesn't seem the right life-cycle for java.sql.Driver.

          I did try thinking about an alternate approach, more in-line with the original code (pre-JDBC 4 autoload) changes where a driver registers itself only, but I then got distracted by other stuff.

          Show
          Daniel John Debrunner added a comment - I think the patch results in code that is brittle, it adds more state about the boot state of the class when it already has a couple of fields trying to manage state. In addition its seems the registering of Derby's embedded driver has become overly complex, some due in part to previous changes, also I think the patch means that the autoloaded driver gets re-used on a re-boot which doesn't seem the right life-cycle for java.sql.Driver. I did try thinking about an alternate approach, more in-line with the original code (pre-JDBC 4 autoload) changes where a driver registers itself only, but I then got distracted by other stuff.
          Hide
          Dyre Tjeldvoll added a comment -

          The last comment seems pretty close to a "-1" for the patch. If that is true, perhaps it would be good to remove 'Patch available' for the time being, at least?

          Show
          Dyre Tjeldvoll added a comment - The last comment seems pretty close to a "-1" for the patch. If that is true, perhaps it would be good to remove 'Patch available' for the time being, at least?
          Hide
          Kathey Marsden added a comment -

          Ramin, are you still working on this issue. It would be good to unassign yourself if you are not.

          Show
          Kathey Marsden added a comment - Ramin, are you still working on this issue. It would be good to unassign yourself if you are not.
          Hide
          Lily Wei added a comment -

          A user (Kuber) on IRC was inquiring about this issue. Rick: Do you have any understanding or insight on what need to be done to fix this bug since you worked on autoloaded driver (DERBY-930)?

          Show
          Lily Wei added a comment - A user (Kuber) on IRC was inquiring about this issue. Rick: Do you have any understanding or insight on what need to be done to fix this bug since you worked on autoloaded driver ( DERBY-930 )?
          Hide
          Rick Hillegas added a comment -

          Hi Lily,

          It has been a long time since I thought about this issue and I no longer remember what the issues are. It is not clear to me what will happen if the driver is unregistered when the engine is shut down. Quite likely, that will break driver autoloading since the JRE autoloads drivers only once. The JRE autoloads the drivers on the very first attempt to resolve a connection URL via DriverManager.getConnection(). Subsequent calls to DriverManager.getConnection() do not trigger autoloading. I can understand that some users want to garbage collect the Derby classes after the engine shuts down. It is likely that other users want autoloading behavior to continue after engine shutdown. It may be possible to satisfy both types of users--and it may not be. An ideal solution would do the following:

          1) Unload all (or almost all) of the Derby classes after engine shutdown.

          2) Preserve the autoloading idiom that resolves a connection url provided that some jarball on the classpath provides an appropriate driver.

          What happens if you explicitly deregister the Derby driver after engine shutdown? What happens if you reregister the Derby driver later on?

          Show
          Rick Hillegas added a comment - Hi Lily, It has been a long time since I thought about this issue and I no longer remember what the issues are. It is not clear to me what will happen if the driver is unregistered when the engine is shut down. Quite likely, that will break driver autoloading since the JRE autoloads drivers only once. The JRE autoloads the drivers on the very first attempt to resolve a connection URL via DriverManager.getConnection(). Subsequent calls to DriverManager.getConnection() do not trigger autoloading. I can understand that some users want to garbage collect the Derby classes after the engine shuts down. It is likely that other users want autoloading behavior to continue after engine shutdown. It may be possible to satisfy both types of users--and it may not be. An ideal solution would do the following: 1) Unload all (or almost all) of the Derby classes after engine shutdown. 2) Preserve the autoloading idiom that resolves a connection url provided that some jarball on the classpath provides an appropriate driver. What happens if you explicitly deregister the Derby driver after engine shutdown? What happens if you reregister the Derby driver later on?
          Hide
          Lily Wei added a comment -

          Hi Rick:
          Thank you so much for the prompt response and leading me to the ideal solution we would like to have - satisfied users who would like to have the 10.1 behavior and users who would like to have the autoloading behavior. Judging from the timeframe of the discussion, this is not an easy problem to get agreement upon. I have to admit I don't totally understand all the complication so any health debate is welcome with the modify solution inspire by Ramin and Dan. The DERBY-2905-1.diff is the proposal patch and Repro2905.java is to show Derby behavior before and after this patch.

          I found this solution solve the following issues
          1. Unload Derby classes after engine shutdown.
          2. Preserve 10.1 behavior and support any future loading of connections
          Disadvantage: It does not preserve the autoloading idiom.

          I am proposing Derby allows support any future loading of connections.

          I did not testing the impact of performance for this patch. Suites.all runs clean.

          Any proposal solution is welcome. Thanks!!!

          Show
          Lily Wei added a comment - Hi Rick: Thank you so much for the prompt response and leading me to the ideal solution we would like to have - satisfied users who would like to have the 10.1 behavior and users who would like to have the autoloading behavior. Judging from the timeframe of the discussion, this is not an easy problem to get agreement upon. I have to admit I don't totally understand all the complication so any health debate is welcome with the modify solution inspire by Ramin and Dan. The DERBY-2905 -1.diff is the proposal patch and Repro2905.java is to show Derby behavior before and after this patch. I found this solution solve the following issues 1. Unload Derby classes after engine shutdown. 2. Preserve 10.1 behavior and support any future loading of connections Disadvantage: It does not preserve the autoloading idiom. I am proposing Derby allows support any future loading of connections. I did not testing the impact of performance for this patch. Suites.all runs clean. Any proposal solution is welcome. Thanks!!!
          Hide
          Rick Hillegas added a comment -

          Hi Lily,

          One way forward may be to separate class-unloading from engine shutdown. I suspect that many users would be satisfied with an explicit api which removed all of the Derby classes, and they would not insist that class-unloading be a side-effect of orderly shutdown. We could document that this new api breaks the autoloading idiom and we could give users instructions on how to reboot the engine and obtain new Derby connections after the unloading. Thanks.

          Show
          Rick Hillegas added a comment - Hi Lily, One way forward may be to separate class-unloading from engine shutdown. I suspect that many users would be satisfied with an explicit api which removed all of the Derby classes, and they would not insist that class-unloading be a side-effect of orderly shutdown. We could document that this new api breaks the autoloading idiom and we could give users instructions on how to reboot the engine and obtain new Derby connections after the unloading. Thanks.
          Hide
          Kathey Marsden added a comment -

          I think shutdown=true with no database specified should shutdown the entire system and deregister the driver as documented.
          http://db.apache.org/derby/docs/dev/ref/rrefattrib16471.html

          I have seen quite a few cases with memory leaks because Derby is loaded in multiple class loaders and leaking because the shutdown is not performed in the same classloader context as the boot and those applications needed to be changed to make sure the shutdown occurred in the proper loader. I don't think it is good to add an additional step as all those applications that have made this change would need to add the additional step and their leaks might reoccur upon upgrade of Derby and the JVM as long as this bug exists.

          Perhaps the new API should be to shutdown while keeping the autoloaded driver. Perhaps a new or modified attribute. I am not very good with names but something like, deregister=<true|false> (only valid with shutdown) and have it default to true.

          Show
          Kathey Marsden added a comment - I think shutdown=true with no database specified should shutdown the entire system and deregister the driver as documented. http://db.apache.org/derby/docs/dev/ref/rrefattrib16471.html I have seen quite a few cases with memory leaks because Derby is loaded in multiple class loaders and leaking because the shutdown is not performed in the same classloader context as the boot and those applications needed to be changed to make sure the shutdown occurred in the proper loader. I don't think it is good to add an additional step as all those applications that have made this change would need to add the additional step and their leaks might reoccur upon upgrade of Derby and the JVM as long as this bug exists. Perhaps the new API should be to shutdown while keeping the autoloaded driver. Perhaps a new or modified attribute. I am not very good with names but something like, deregister=<true|false> (only valid with shutdown) and have it default to true.
          Hide
          Knut Anders Hatlen added a comment -

          Adding a deregister attribute to the URL when shutting down the system sounds like a good idea to me. I think true would be a reasonable default, as that would make the behaviour consistent with the documentation. I don't think that it would break the autoloading idiom any more than it already is in this case. If we do a getConnection() after a driver shutdown, without doing Class.forName(...).newInstance() in between, we get this exception with the current code:

          Exception in thread "main" java.sql.SQLException: org.apache.derby.jdbc.EmbeddedDriver is not registered with the JDBC driver manager
          at org.apache.derby.jdbc.AutoloadedDriver.getDriverModule(AutoloadedDriver.java:186)
          at org.apache.derby.jdbc.AutoloadedDriver.connect(AutoloadedDriver.java:119)
          at java.sql.DriverManager.getConnection(DriverManager.java:620)
          at java.sql.DriverManager.getConnection(DriverManager.java:222)

          If we make shutdown deregister AutoloadedDriver, we'd probably get a "No suitable driver" error instead, but I don't think that's any worse than what we have now.

          Show
          Knut Anders Hatlen added a comment - Adding a deregister attribute to the URL when shutting down the system sounds like a good idea to me. I think true would be a reasonable default, as that would make the behaviour consistent with the documentation. I don't think that it would break the autoloading idiom any more than it already is in this case. If we do a getConnection() after a driver shutdown, without doing Class.forName(...).newInstance() in between, we get this exception with the current code: Exception in thread "main" java.sql.SQLException: org.apache.derby.jdbc.EmbeddedDriver is not registered with the JDBC driver manager at org.apache.derby.jdbc.AutoloadedDriver.getDriverModule(AutoloadedDriver.java:186) at org.apache.derby.jdbc.AutoloadedDriver.connect(AutoloadedDriver.java:119) at java.sql.DriverManager.getConnection(DriverManager.java:620) at java.sql.DriverManager.getConnection(DriverManager.java:222) If we make shutdown deregister AutoloadedDriver, we'd probably get a "No suitable driver" error instead, but I don't think that's any worse than what we have now.
          Hide
          Lily Wei added a comment -

          Thanks Knut and Kathey. I think shutdown=true should shutdown the entire system and deregister the driver as document. DERBY-2905-2.diff is implementing that. In AutoloadDriver, the _autoloadedDriver is to keep track of the AutoloadedDriver when getConnection is called the first time and did not get trigger on the following calls. This implementation did not truly implement deregister=true other than introduce the attribute. I am posting the patch to make sure we agree on the proposal and whether I am on the right trace of fixing this issue.

          To summary the current proposal:
          shutdown=true will shutdown the entire system and deregister the driver as document. On top of that, we will introduce deregister attribute. If deregister=false, we will keep the autoloading idiom and allow further getConnection after that. The default behavior for shutdown will be setting deregister=true.

          Some note about the patch:
          Driver20.stop will deregister the driver as document when shutdown attribute is set. No change is needed.

          EmbeddedDataSource.java If AutoloadedDriver has been deregister from previous shutdown, just get the current driver.

          Attribute.java introduce deregister attribute

          AutoloadedDriver: handle register and deregister driver. i.e. AutoloadedDriver, Driver40(other if using different version of jvm) Need to know whether it is register driver the first time or after AutoloadedDriver has been initiated. I am using _driverModule to keep track of the derby driver and _autoloadedDriver to keep track of the AutoloadedDriver object.

          What is not implemented? The work of deregister=false.

          Suites.all runs clean.

          Show
          Lily Wei added a comment - Thanks Knut and Kathey. I think shutdown=true should shutdown the entire system and deregister the driver as document. DERBY-2905 -2.diff is implementing that. In AutoloadDriver, the _autoloadedDriver is to keep track of the AutoloadedDriver when getConnection is called the first time and did not get trigger on the following calls. This implementation did not truly implement deregister=true other than introduce the attribute. I am posting the patch to make sure we agree on the proposal and whether I am on the right trace of fixing this issue. To summary the current proposal: shutdown=true will shutdown the entire system and deregister the driver as document. On top of that, we will introduce deregister attribute. If deregister=false, we will keep the autoloading idiom and allow further getConnection after that. The default behavior for shutdown will be setting deregister=true. Some note about the patch: Driver20.stop will deregister the driver as document when shutdown attribute is set. No change is needed. EmbeddedDataSource.java If AutoloadedDriver has been deregister from previous shutdown, just get the current driver. Attribute.java introduce deregister attribute AutoloadedDriver: handle register and deregister driver. i.e. AutoloadedDriver, Driver40(other if using different version of jvm) Need to know whether it is register driver the first time or after AutoloadedDriver has been initiated. I am using _driverModule to keep track of the derby driver and _autoloadedDriver to keep track of the AutoloadedDriver object. What is not implemented? The work of deregister=false. Suites.all runs clean.
          Hide
          Kathey Marsden added a comment -

          I looked the DERBY-2905-2.diff patch. I am not so clear on all the autoloading implications, so it would be good if someone else could take a look too. Here are my comments.

          In EmbeddedDataSource findDriver() I think we should just Call DriverManager.getDriver(url) once and save the value. to improve performance and make the code clearer. I was sort of surprised that finding the driver is done for every connection rather than on instantiating the DataSource, but that was a preexisting situation.

          In AutoloadedDriver I wonder if we can avoid having the new activeautoloadeddriver boolean and just use the _autoloadedDriver and check if it was null.

          In unregisterDriverManager, I wonder if _autoLoadedDriver should be set to null and also if the driverModule shouild be deregistered even if the autoloaded driver is registered.

          In the test we should not import ClientDataSource directly and instead use JDBCDataSource.getDataSource() and also should not assume the driver is Driver40 as it might be Driver30 etc.

          Again I am pretty fuzzy on this autoloading stuff so it would be good if someone else could look.

          Show
          Kathey Marsden added a comment - I looked the DERBY-2905 -2.diff patch. I am not so clear on all the autoloading implications, so it would be good if someone else could take a look too. Here are my comments. In EmbeddedDataSource findDriver() I think we should just Call DriverManager.getDriver(url) once and save the value. to improve performance and make the code clearer. I was sort of surprised that finding the driver is done for every connection rather than on instantiating the DataSource, but that was a preexisting situation. In AutoloadedDriver I wonder if we can avoid having the new activeautoloadeddriver boolean and just use the _autoloadedDriver and check if it was null. In unregisterDriverManager, I wonder if _autoLoadedDriver should be set to null and also if the driverModule shouild be deregistered even if the autoloaded driver is registered. In the test we should not import ClientDataSource directly and instead use JDBCDataSource.getDataSource() and also should not assume the driver is Driver40 as it might be Driver30 etc. Again I am pretty fuzzy on this autoloading stuff so it would be good if someone else could look.
          Hide
          Lily Wei added a comment -

          Thanks Kathey. Yes, it will be great to have someone else look at the patch too. I upload DERBY-2905_3.diff based on your comments.

          In EmbeddedDataSourceFindDriver(), DriverManager.getDriver(url) is saved to improve performance. Having finding driver in DataSource instead of every connection could improve performance and oo design. Is there any other impact to Derby system?

          I tried to used _autoloadedDriver==null to handle AutoloadedDriver.registerDriverModule smf AutoloadedDriver.unregisterDriverModule. I am trouble by _driverModule not being set at all the appropriate time causing some of the getConnection get "not suitable driver" error either after Class.forName(url).newInstance(). I suggest to keep the activeautoloadeddriver Boolean to remember the very first time AutoloadedDriver is initiated and getting deregister from DriverManager.

          The _autoloadedDriver is set to null in AutoloadedDriver.unregisterDriverModule.

          In the test, the import of ClientDataSource is taking out and we test unregisterDriverModule on Driver40, Driver30, and Driver20.

          Suites.all and derbyall run clean. The code is ready to review. It will be nice to see more review. I am writing the code for deregister attribute on shutdown time.

          Show
          Lily Wei added a comment - Thanks Kathey. Yes, it will be great to have someone else look at the patch too. I upload DERBY-2905 _3.diff based on your comments. In EmbeddedDataSourceFindDriver(), DriverManager.getDriver(url) is saved to improve performance. Having finding driver in DataSource instead of every connection could improve performance and oo design. Is there any other impact to Derby system? I tried to used _autoloadedDriver==null to handle AutoloadedDriver.registerDriverModule smf AutoloadedDriver.unregisterDriverModule. I am trouble by _driverModule not being set at all the appropriate time causing some of the getConnection get "not suitable driver" error either after Class.forName(url).newInstance(). I suggest to keep the activeautoloadeddriver Boolean to remember the very first time AutoloadedDriver is initiated and getting deregister from DriverManager. The _autoloadedDriver is set to null in AutoloadedDriver.unregisterDriverModule. In the test, the import of ClientDataSource is taking out and we test unregisterDriverModule on Driver40, Driver30, and Driver20. Suites.all and derbyall run clean. The code is ready to review. It will be nice to see more review. I am writing the code for deregister attribute on shutdown time.
          Hide
          Lily Wei added a comment -

          Regressions ran ok, committed DERBY-2905_3.diff as svn 1063996 for the first part of fix.

          Show
          Lily Wei added a comment - Regressions ran ok, committed DERBY-2905 _3.diff as svn 1063996 for the first part of fix.
          Hide
          Lily Wei added a comment -

          The DERBY-2905_part2_1.diff patch adds the attribute deregistere. The default behavior on shutdown is to deregister AutoloaderDriver. If users specify deregister=false attribute at shutdown, Derby will not deregister AutoloadedDriver. And, getConnection will get a new connection. I still need to write releaseNote.html for this issue.

          I had run more tests on the side. I am still in the middle of getting them into AutoloadTest. The code is ready for review especially on adding new error message part. I am not sure we should add new error message here. Any suggestion is welcome.

          Running suites.all and derbyall now.

          Show
          Lily Wei added a comment - The DERBY-2905 _part2_1.diff patch adds the attribute deregistere. The default behavior on shutdown is to deregister AutoloaderDriver. If users specify deregister=false attribute at shutdown, Derby will not deregister AutoloadedDriver. And, getConnection will get a new connection. I still need to write releaseNote.html for this issue. I had run more tests on the side. I am still in the middle of getting them into AutoloadTest. The code is ready for review especially on adding new error message part. I am not sure we should add new error message here. Any suggestion is welcome. Running suites.all and derbyall now.
          Hide
          Rick Hillegas added a comment -

          Attaching ww.java. This patch shows an odd side-effect of the 1063996 submission. Before that submission, reloading the engine via Class.forName(...) resulted in the AutoloadedDriver being registered. After the 1063996 checkin, the same sequence of steps results in Driver40 being registered. I think it would be better if DriverManager.getDriver() always returned an AutoloadedDriver. I have some misgivings that supporting two paths here is going to give rise to mischief.

          Show
          Rick Hillegas added a comment - Attaching ww.java. This patch shows an odd side-effect of the 1063996 submission. Before that submission, reloading the engine via Class.forName(...) resulted in the AutoloadedDriver being registered. After the 1063996 checkin, the same sequence of steps results in Driver40 being registered. I think it would be better if DriverManager.getDriver() always returned an AutoloadedDriver. I have some misgivings that supporting two paths here is going to give rise to mischief.
          Hide
          Lily Wei added a comment -

          Thanks Rick. I am uploading patch DERBY_2905_part2_2.diff to change the reloading the engine via Class.forName(…) resulted in the AutoloadedDriver registered. This patch also provide the deregister attribute. The default behavior for shutdown=true is to set deregister=true. AutoloadedDriver will not be in DriverManager. If users set deregister=false, the AutoloadDriver will not be deregister from DriverManager. The AutoloadTest create test cases like: default shutdown behavior, deregister=true, deregister=false and also wrong name of deregister attribute. The patch is ready for review.
          Suites.all and derby passed before fixing the reload engine resulted in AutoloadedDriver begin registered. I will run the tests again.

          Show
          Lily Wei added a comment - Thanks Rick. I am uploading patch DERBY_2905_part2_2.diff to change the reloading the engine via Class.forName(…) resulted in the AutoloadedDriver registered. This patch also provide the deregister attribute. The default behavior for shutdown=true is to set deregister=true. AutoloadedDriver will not be in DriverManager. If users set deregister=false, the AutoloadDriver will not be deregister from DriverManager. The AutoloadTest create test cases like: default shutdown behavior, deregister=true, deregister=false and also wrong name of deregister attribute. The patch is ready for review. Suites.all and derby passed before fixing the reload engine resulted in AutoloadedDriver begin registered. I will run the tests again.
          Hide
          Rick Hillegas added a comment -

          Thanks, Lily. I have applied the patch and verified that it fixes the behavior shown by the ww.java test case. Thanks.

          Show
          Rick Hillegas added a comment - Thanks, Lily. I have applied the patch and verified that it fixes the behavior shown by the ww.java test case. Thanks.
          Hide
          Lily Wei added a comment -

          Thanks, Rick. The patch is check-in svn 1068842. If the reloading the engine via Class.forName(...).newInstance resulted in AutoloadedDriver registered does not load the desire AutoloadedDriver, please feel free to change the code. Before resync the latest change, Suites.all and derbyall passed. Hope this help, thanks!

          Show
          Lily Wei added a comment - Thanks, Rick. The patch is check-in svn 1068842. If the reloading the engine via Class.forName(...).newInstance resulted in AutoloadedDriver registered does not load the desire AutoloadedDriver, please feel free to change the code. Before resync the latest change, Suites.all and derbyall passed. Hope this help, thanks!
          Hide
          Dag H. Wanvik added a comment -

          Lily, the latest patch (DERBY_2905_part2_2) doesn't have the Apache grant flag set, so I think you should reattach it, so it's legally in order

          Show
          Dag H. Wanvik added a comment - Lily, the latest patch (DERBY_2905_part2_2) doesn't have the Apache grant flag set, so I think you should reattach it, so it's legally in order
          Hide
          Lily Wei added a comment -

          Oops! You are absolutely right. I reattach DERBY-2905_part2_2_1.diff and check the Apache grant flag. Thank you so much for catching that, Dag.

          Show
          Lily Wei added a comment - Oops! You are absolutely right. I reattach DERBY-2905 _part2_2_1.diff and check the Apache grant flag. Thank you so much for catching that, Dag.
          Show
          Knut Anders Hatlen added a comment - Looks like there were two new failures in the Tinderbox after the latest commit: http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/1068846-org.apache.derbyTesting.functionTests.suites.All_diff.txt
          Hide
          Dag H. Wanvik added a comment -

          I read through the latest patch, some small nits and possible a test bug:

          • In AutoloadedDriver, typo in new comment:
            > //This flag is set is deregister attribute is set by user,
            • "if" instead of second "is"
              > //default is true (DERBY-2905)

          Better insert space after "//" too, for readability.
          If I understand this correctly, if the default is "true", a more
          correct comment would perhaps be:

          "This flag is true unless the deregister attribute has been set to
          false by the user (DERBY-2905)."

          • InternalDriver, new comment:

          > // DERBY-2905, allow users to provide deregister attribute to
          > // left AutoloadedDriver in DriverManager, default value is true

          "left AutoloadedDriver" -> "leave AutoloadedDriver registered"

          • AutoloadTest

          Typo in comment:

          > * the AutoloadDriver is dergistered from DriverManager.

              • "dergistered"

          This looks like a test bug:

          > //Case 1: Test the deregister attribute error
          > JDBCDataSource.setBeanProperty(ds, "connectionAttributes",
          > "shutdown=true;deregiste=false");

            • missing "r" in "deregister" ?

          The extraneous blank inside "true" below looks suspicious, not sure
          if it works as expected:

          > JDBCDataSource.setBeanProperty(ds, "connectionAttributes",
          > "shutdown=tru e;deregister=false");

              • "tru e"
          Show
          Dag H. Wanvik added a comment - I read through the latest patch, some small nits and possible a test bug: In AutoloadedDriver, typo in new comment: > //This flag is set is deregister attribute is set by user, "if" instead of second "is" > //default is true ( DERBY-2905 ) Better insert space after "//" too, for readability. If I understand this correctly, if the default is "true", a more correct comment would perhaps be: "This flag is true unless the deregister attribute has been set to false by the user ( DERBY-2905 )." InternalDriver, new comment: > // DERBY-2905 , allow users to provide deregister attribute to > // left AutoloadedDriver in DriverManager, default value is true "left AutoloadedDriver" -> "leave AutoloadedDriver registered" AutoloadTest Typo in comment: > * the AutoloadDriver is dergistered from DriverManager. "dergistered" This looks like a test bug: > //Case 1: Test the deregister attribute error > JDBCDataSource.setBeanProperty(ds, "connectionAttributes", > "shutdown=true;deregiste=false"); missing "r" in "deregister" ? The extraneous blank inside "true" below looks suspicious, not sure if it works as expected: > JDBCDataSource.setBeanProperty(ds, "connectionAttributes", > "shutdown=tru e;deregister=false"); "tru e"
          Hide
          Lily Wei added a comment -

          Thanks Dag for reviewing the patch.

          I read through the latest patch, some small nits and possible a test bug:

          • In AutoloadedDriver, typo in new comment:
            > //This flag is set is deregister attribute is set by user,
            • "if" instead of second "is"
              > //default is true (DERBY-2905)
              >>I change the typo in DERBY-2905_part2_2_2.diff

          Better insert space after "//" too, for readability.
          If I understand this correctly, if the default is "true", a more
          correct comment would perhaps be:

          "This flag is true unless the deregister attribute has been set to
          false by the user (DERBY-2905)."
          >>Add the space and change the commend.

          • InternalDriver, new comment:

          > // DERBY-2905, allow users to provide deregister attribute to
          > // left AutoloadedDriver in DriverManager, default value is true

          "left AutoloadedDriver" -> "leave AutoloadedDriver registered"
          >>Change the comment in the new patch.

          • AutoloadTest

          Typo in comment:

          > * the AutoloadDriver is dergistered from DriverManager.

              • "dergistered"

          >>Change in the patch
          This looks like a test bug:

          > //Case 1: Test the deregister attribute error
          > JDBCDataSource.setBeanProperty(ds, "connectionAttributes",
          > "shutdown=true;deregiste=false");

            • missing "r" in "deregister" ?
              >>This is the desire test. The typo is by design. I believe the behavior is expected.
              When I run in ij, I got:
              ij> connect 'jdbc:derby:system/wombat;shutdown=true;deregiste=false';
              URL Attribute [deregiste=false]
              Attribute is unknown to Derby.
              ERROR 08006: Database 'system/wombat' shutdown.

          The extraneous blank inside "true" below looks suspicious, not sure
          if it works as expected:

          > JDBCDataSource.setBeanProperty(ds, "connectionAttributes",
          > "shutdown=tru e;deregister=false");

              • "tru e"
                >>The space is taking out with the new patch.
          Show
          Lily Wei added a comment - Thanks Dag for reviewing the patch. I read through the latest patch, some small nits and possible a test bug: In AutoloadedDriver, typo in new comment: > //This flag is set is deregister attribute is set by user, "if" instead of second "is" > //default is true ( DERBY-2905 ) >>I change the typo in DERBY-2905 _part2_2_2.diff Better insert space after "//" too, for readability. If I understand this correctly, if the default is "true", a more correct comment would perhaps be: "This flag is true unless the deregister attribute has been set to false by the user ( DERBY-2905 )." >>Add the space and change the commend. InternalDriver, new comment: > // DERBY-2905 , allow users to provide deregister attribute to > // left AutoloadedDriver in DriverManager, default value is true "left AutoloadedDriver" -> "leave AutoloadedDriver registered" >>Change the comment in the new patch. AutoloadTest Typo in comment: > * the AutoloadDriver is dergistered from DriverManager. "dergistered" >>Change in the patch This looks like a test bug: > //Case 1: Test the deregister attribute error > JDBCDataSource.setBeanProperty(ds, "connectionAttributes", > "shutdown=true;deregiste=false"); missing "r" in "deregister" ? >>This is the desire test. The typo is by design. I believe the behavior is expected. When I run in ij, I got: ij> connect 'jdbc:derby:system/wombat;shutdown=true;deregiste=false'; URL Attribute [deregiste=false] Attribute is unknown to Derby. ERROR 08006: Database 'system/wombat' shutdown. The extraneous blank inside "true" below looks suspicious, not sure if it works as expected: > JDBCDataSource.setBeanProperty(ds, "connectionAttributes", > "shutdown=tru e;deregister=false"); "tru e" >>The space is taking out with the new patch.
          Hide
          Lily Wei added a comment -

          Hi Knut:
          When I run it with DERBY-2905_part2_2_2.diff patch. I did not see the two
          new failures.
          This is the my results. I will keep on looking. Please see the following:
          $ java -Dderby.tests.trace=true junit.textui.TestRunner org.apache.derbyTesting
          .functionTests.tests.jdbc4.Driver40Test
          .
          test_jdbc4_1 used 6 ms .
          test_jdbc4_1 used 55 ms
          Time: 12.188

          OK (2 tests)
          $ java -Dderby.tests.trace=true junit.textui.TestRunner org.apache.derbyTesting
          .functionTests.tests.jdbcapi.DriverTest
          .
          testDriverCompliantVersion used 194 ms .
          testAcceptsURL used 4 ms .
          testEmbeddedAttributes used 20034 ms .
          testClientAttributes used 4 ms .
          testClientURL used 6 ms .
          testDbNameWithSpaces used 6644 ms .
          testDriverCompliantVersion used 140 ms .
          testAcceptsURL used 4 ms .
          testEmbeddedAttributes used 13573 ms .
          testClientAttributes used 6817 ms .
          testClientURL used 6394 ms .
          testDbNameWithSpaces used 6604 ms
          Time: 71.389

          OK (12 tests)

          Thanks,
          Lily

          Show
          Lily Wei added a comment - Hi Knut: When I run it with DERBY-2905 _part2_2_2.diff patch. I did not see the two new failures. This is the my results. I will keep on looking. Please see the following: $ java -Dderby.tests.trace=true junit.textui.TestRunner org.apache.derbyTesting .functionTests.tests.jdbc4.Driver40Test . test_jdbc4_1 used 6 ms . test_jdbc4_1 used 55 ms Time: 12.188 OK (2 tests) $ java -Dderby.tests.trace=true junit.textui.TestRunner org.apache.derbyTesting .functionTests.tests.jdbcapi.DriverTest . testDriverCompliantVersion used 194 ms . testAcceptsURL used 4 ms . testEmbeddedAttributes used 20034 ms . testClientAttributes used 4 ms . testClientURL used 6 ms . testDbNameWithSpaces used 6644 ms . testDriverCompliantVersion used 140 ms . testAcceptsURL used 4 ms . testEmbeddedAttributes used 13573 ms . testClientAttributes used 6817 ms . testClientURL used 6394 ms . testDbNameWithSpaces used 6604 ms Time: 71.389 OK (12 tests) Thanks, Lily
          Hide
          Knut Anders Hatlen added a comment -

          Hi Lily,

          I applied the part2_2_2 patch, and I still see the failure in jdbc4.Driver40Test. Or to be precise, I see the failure when I run jdbc4._Suite. I do not see the failure when I run Driver40Test separately. Perhaps the test behaves differently when the driver has been loaded before the test starts?

          Show
          Knut Anders Hatlen added a comment - Hi Lily, I applied the part2_2_2 patch, and I still see the failure in jdbc4.Driver40Test. Or to be precise, I see the failure when I run jdbc4._Suite. I do not see the failure when I run Driver40Test separately. Perhaps the test behaves differently when the driver has been loaded before the test starts?
          Hide
          Lily Wei added a comment -

          Thanks Knut. You are right, the test does behave differently when the driver has been loaded and shutdown has been issued before the test starts. When we first start embedded engine, AutoloadedDriver40 is created by Derby and loaded in DriverManager. With the current code, reloading engine after shutdown via for.ClassName(…).newInstance will result AutoloadedDriver loaded in DriverManager. Jdbc4.Driver40Test and jdbcapi.DriverTest both assume AutoloadedDriver40 is in DriverManager when JDBC.vmSupportsJDBC4() is true. I am attaching DERBY-2905_part2_2_3.diff patch. This patch assumes the current AutoloadedDriver.registerDriverModule() behavior is correct. If this is not a correct assumption, please feel free to change the code. I did run jdbc4._Suite and jdbcapi._Suite and they are passing on my machine. Please review the code. If it is okay with everybody, I can check-in when I get home. I am currently in HongKong airport. Happy Chinese New Year.

          Show
          Lily Wei added a comment - Thanks Knut. You are right, the test does behave differently when the driver has been loaded and shutdown has been issued before the test starts. When we first start embedded engine, AutoloadedDriver40 is created by Derby and loaded in DriverManager. With the current code, reloading engine after shutdown via for.ClassName(…).newInstance will result AutoloadedDriver loaded in DriverManager. Jdbc4.Driver40Test and jdbcapi.DriverTest both assume AutoloadedDriver40 is in DriverManager when JDBC.vmSupportsJDBC4() is true. I am attaching DERBY-2905 _part2_2_3.diff patch. This patch assumes the current AutoloadedDriver.registerDriverModule() behavior is correct. If this is not a correct assumption, please feel free to change the code. I did run jdbc4._Suite and jdbcapi._Suite and they are passing on my machine. Please review the code. If it is okay with everybody, I can check-in when I get home. I am currently in HongKong airport. Happy Chinese New Year.
          Hide
          Rick Hillegas added a comment -

          I have looked at the DERBY-2905_part2_2_3.diff patch. It does indeed make Driver40Test succeed. But that is because it disables the tests which Driver40Test is trying to perform. The actual functionality remains broken. After reloading the engine on Java 6 or higher, the registered driver should be an AutoloadedDriver40. That is because that is the driver which contains the getParentLogger() method required by JDBC 4.1. The getParentLogger() method does not exist in AutoloadedDriver.

          Here is the output of ww.java after applying this patch:

          org.apache.derby.jdbc.AutoloadedDriver40@42b307f0
          org.apache.derby.jdbc.AutoloadedDriver@236acdd1

          Both lines should report an AutoloadedDriver40.

          Show
          Rick Hillegas added a comment - I have looked at the DERBY-2905 _part2_2_3.diff patch. It does indeed make Driver40Test succeed. But that is because it disables the tests which Driver40Test is trying to perform. The actual functionality remains broken. After reloading the engine on Java 6 or higher, the registered driver should be an AutoloadedDriver40. That is because that is the driver which contains the getParentLogger() method required by JDBC 4.1. The getParentLogger() method does not exist in AutoloadedDriver. Here is the output of ww.java after applying this patch: org.apache.derby.jdbc.AutoloadedDriver40@42b307f0 org.apache.derby.jdbc.AutoloadedDriver@236acdd1 Both lines should report an AutoloadedDriver40.
          Hide
          Rick Hillegas added a comment -

          I think I lied to you in my comment on 2011-02-08. The DERBY_2905_part2_2.diff patch seems to have changed the reloaded driver from a Driver40 into an AutoloadedDriver. I saw the AutoloadedDriver bit but I missed the fact that it did not end in 40. So that patch made the situation worse. -1 to that patch now.

          Show
          Rick Hillegas added a comment - I think I lied to you in my comment on 2011-02-08. The DERBY_2905_part2_2.diff patch seems to have changed the reloaded driver from a Driver40 into an AutoloadedDriver. I saw the AutoloadedDriver bit but I missed the fact that it did not end in 40. So that patch made the situation worse. -1 to that patch now.
          Hide
          Kathey Marsden added a comment -

          What user functionality is broken with the DERBY_2905_part2_2.diff? I have not seen any failures on JDK 1.6. Is it JDK 1.7 specific related functionality of getParentLogger when the AutoloadedDriver is reloaded after shutdown? Since there is no regression to previous releases and no JDK 1.7 released. I think it would make sense to file a separate issue for the JDK 1.7 issue and work on that separately as part of the JDBC 4.1 work.

          I think in general the Autoloader code was designed with the assumption that it would never be unloaded with all the references in DataSources and such, so it was quite tricky I think for Lily to figure out how to get the updates in all the right places, but it does work and is a great improvement over the current state. Maybe the addition of AutoloadedDriver40 warrants a refactoring to remove that assumption.

          Show
          Kathey Marsden added a comment - What user functionality is broken with the DERBY_2905_part2_2.diff? I have not seen any failures on JDK 1.6. Is it JDK 1.7 specific related functionality of getParentLogger when the AutoloadedDriver is reloaded after shutdown? Since there is no regression to previous releases and no JDK 1.7 released. I think it would make sense to file a separate issue for the JDK 1.7 issue and work on that separately as part of the JDBC 4.1 work. I think in general the Autoloader code was designed with the assumption that it would never be unloaded with all the references in DataSources and such, so it was quite tricky I think for Lily to figure out how to get the updates in all the right places, but it does work and is a great improvement over the current state. Maybe the addition of AutoloadedDriver40 warrants a refactoring to remove that assumption.
          Hide
          Rick Hillegas added a comment -

          Hi Kathey. Yes the user-visible functionality is visible on JDK 7. The problem also appears on JDK 6 in the form that the registered driver is different after reloading the engine. That will cause tech support a lot of mischief down the road because it will give rise to subtle problems which depend on whether an action is performed before or after reloading the engine. I have provided a test case which shows the problem of different drivers on JDK 6: ww.java. -1 to your suggestion that this is a JDBC 4.1 problem.

          Show
          Rick Hillegas added a comment - Hi Kathey. Yes the user-visible functionality is visible on JDK 7. The problem also appears on JDK 6 in the form that the registered driver is different after reloading the engine. That will cause tech support a lot of mischief down the road because it will give rise to subtle problems which depend on whether an action is performed before or after reloading the engine. I have provided a test case which shows the problem of different drivers on JDK 6: ww.java. -1 to your suggestion that this is a JDBC 4.1 problem.
          Hide
          Rick Hillegas added a comment -

          I take part of that back. Perhaps you cannot see this with ww.java on JDK 6. In any event, I have spent a lot of time implementing JDBC 4.1 and I do not appreciate the problem being kicked over to me.

          Show
          Rick Hillegas added a comment - I take part of that back. Perhaps you cannot see this with ww.java on JDK 6. In any event, I have spent a lot of time implementing JDBC 4.1 and I do not appreciate the problem being kicked over to me.
          Hide
          Kathey Marsden added a comment -

          I am not kicking it over to you personally but rather suggesting we file a separate issue and whomever is interested in fixing it can, but as you recall this issue, DERBY-2905 was a regression introduced by your original Autoloader work. Several times your work has introduced outright regressions including this one that you have left to others to fix. In this case of this regression, Lily has worked hard to fix the regression and made her plans clear over the course of development. I am sure it would have been helpful to communicate more about potential conflicts during that process.

          Show
          Kathey Marsden added a comment - I am not kicking it over to you personally but rather suggesting we file a separate issue and whomever is interested in fixing it can, but as you recall this issue, DERBY-2905 was a regression introduced by your original Autoloader work. Several times your work has introduced outright regressions including this one that you have left to others to fix. In this case of this regression, Lily has worked hard to fix the regression and made her plans clear over the course of development. I am sure it would have been helpful to communicate more about potential conflicts during that process.
          Hide
          Lily Wei added a comment -

          +1 on communicating more in turn of conflicts and potential fix. I will be more than happy to keep working on resolving getParentLogger issue for AutoloadedDriver. I will definitely appreciate Rick's suggestion for how to proceed.
          As far as I can see, we need to create AutoloadedDriver40 in AutoloadedDriver.registerDriverModule. Something like this:
          if (_autoloadedDriver == null) {
          //Support JDBC 4 or higher (DERBY-2905)
          if (HAVE_DRIVER && HAVE_SQLXML)

          { AutoloadedDriver40.reloadAutoloadedDriver40(); }

          However, I am having compiler error on the line "AutoloadedDriver40.reloadAutoloadedDriver40 on reflect, invoke etc.

          Show
          Lily Wei added a comment - +1 on communicating more in turn of conflicts and potential fix. I will be more than happy to keep working on resolving getParentLogger issue for AutoloadedDriver. I will definitely appreciate Rick's suggestion for how to proceed. As far as I can see, we need to create AutoloadedDriver40 in AutoloadedDriver.registerDriverModule. Something like this: if (_autoloadedDriver == null) { //Support JDBC 4 or higher ( DERBY-2905 ) if (HAVE_DRIVER && HAVE_SQLXML) { AutoloadedDriver40.reloadAutoloadedDriver40(); } However, I am having compiler error on the line "AutoloadedDriver40.reloadAutoloadedDriver40 on reflect, invoke etc.
          Hide
          Rick Hillegas added a comment -

          Kathey, no one is faulting Lily's hard work or cleverness here. She has done a great job. But this is tricky code. I think you should avoid personalizing the issue. It is not helpful. Perhaps we should have a friendly chat over a couple beers next week. The -1 stands.

          Show
          Rick Hillegas added a comment - Kathey, no one is faulting Lily's hard work or cleverness here. She has done a great job. But this is tricky code. I think you should avoid personalizing the issue. It is not helpful. Perhaps we should have a friendly chat over a couple beers next week. The -1 stands.
          Hide
          Rick Hillegas added a comment -

          Hi Lily,

          The following code snippet may help move this forward:

          public static AutoloadedDriver makeAutoloadedDriver()
          {
          try

          { return (AutoloadedDriver) Class.forName( "org.apache.derby.jdbc.AutoloadedDriver40" ).newInstance(); }

          catch (Throwable t) {}

          return new AutoloadedDriver();
          }

          Hope that's useful.

          Regards,
          -Rick

          Show
          Rick Hillegas added a comment - Hi Lily, The following code snippet may help move this forward: public static AutoloadedDriver makeAutoloadedDriver() { try { return (AutoloadedDriver) Class.forName( "org.apache.derby.jdbc.AutoloadedDriver40" ).newInstance(); } catch (Throwable t) {} return new AutoloadedDriver(); } Hope that's useful. Regards, -Rick
          Hide
          Kathey Marsden added a comment -

          I think there is no reason to veto the patch. The fix is a marked improvement in behavior, fixes the regression and causes no functional regression from a released version. Let's file a new issue for the minor getParentLogger after shutdown issue, fix that and move forward.

          Show
          Kathey Marsden added a comment - I think there is no reason to veto the patch. The fix is a marked improvement in behavior, fixes the regression and causes no functional regression from a released version. Let's file a new issue for the minor getParentLogger after shutdown issue, fix that and move forward.
          Hide
          Rick Hillegas added a comment -

          I think that Lily is going to very close to fixing this. Let's help her do that.

          Show
          Rick Hillegas added a comment - I think that Lily is going to very close to fixing this. Let's help her do that.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-2905-01-aa-fixAutoloadedDriverReload.diff. This appears to fix the problem with Driver40Test. I will run regression tests.

          This patch applies the solution from my previous comment.

          Touches the following file:

          M java/engine/org/apache/derby/jdbc/AutoloadedDriver.java

          Show
          Rick Hillegas added a comment - Attaching derby-2905-01-aa-fixAutoloadedDriverReload.diff. This appears to fix the problem with Driver40Test. I will run regression tests. This patch applies the solution from my previous comment. Touches the following file: M java/engine/org/apache/derby/jdbc/AutoloadedDriver.java
          Hide
          Kathey Marsden added a comment -

          Sounds good and thank you for helping. That, I think is generally is a better approach than rejecting a valuable contribution outright. Lily filed DERBY-5029 to track the issue moving forward.

          Show
          Kathey Marsden added a comment - Sounds good and thank you for helping. That, I think is generally is a better approach than rejecting a valuable contribution outright. Lily filed DERBY-5029 to track the issue moving forward.
          Hide
          Lily Wei added a comment -

          Add release notes.

          Show
          Lily Wei added a comment - Add release notes.
          Hide
          Lily Wei added a comment -

          Add release notes.

          Show
          Lily Wei added a comment - Add release notes.
          Hide
          Dag H. Wanvik added a comment -

          Release notes look good. +1.

          Show
          Dag H. Wanvik added a comment - Release notes look good. +1.
          Hide
          Knut Anders Hatlen added a comment -

          Attaching an updated release note.

          Made the summary a one-liner ("System shutdown now deregisters the embedded JDBC driver") and incorporated the information in the original summary into the other sections.

          Show
          Knut Anders Hatlen added a comment - Attaching an updated release note. Made the summary a one-liner ("System shutdown now deregisters the embedded JDBC driver") and incorporated the information in the original summary into the other sections.
          Hide
          Rick Hillegas added a comment -

          Adjusted the release note. Further adjustment may be needed depending on the answer to the question which I just posted on DERBY-5043.

          Show
          Rick Hillegas added a comment - Adjusted the release note. Further adjustment may be needed depending on the answer to the question which I just posted on DERBY-5043 .
          Hide
          Kathey Marsden added a comment -

          I definitely think it was valuable to have information on the deregister attribute in the release note, because it was added as part of this change and especially because it might not make the 10.8 documentation, so +1 to adjusting the releaseNote to include it once again.

          Show
          Kathey Marsden added a comment - I definitely think it was valuable to have information on the deregister attribute in the release note, because it was added as part of this change and especially because it might not make the 10.8 documentation, so +1 to adjusting the releaseNote to include it once again.
          Hide
          Rick Hillegas added a comment -

          We should document this attribute in the user guides unless this is an experimental feature which we intend to change in a future release. I will adjust the release note after I have something I can point at. I will hold off building an RC until after this documentation is complete.

          Show
          Rick Hillegas added a comment - We should document this attribute in the user guides unless this is an experimental feature which we intend to change in a future release. I will adjust the release note after I have something I can point at. I will hold off building an RC until after this documentation is complete.
          Hide
          Kathey Marsden added a comment -

          We definitely want to keep the functionality. The only thought of change that came to mind for me in this exchange is that perhaps the double negative might be confusing. The only possible alternative I could think of would be something like keepregistered=true instead of deregister=false, but maybe it is fine the way it is as that would be kind of wordy and we would need to change it quickly.

          Show
          Kathey Marsden added a comment - We definitely want to keep the functionality. The only thought of change that came to mind for me in this exchange is that perhaps the double negative might be confusing. The only possible alternative I could think of would be something like keepregistered=true instead of deregister=false, but maybe it is fine the way it is as that would be kind of wordy and we would need to change it quickly.
          Hide
          Rick Hillegas added a comment -

          I don't have strong religion about what to call this attribute. I think that deregister=true is fine. I would prefer to leave it alone now that Kim is in the middle of documenting the feature.

          Show
          Rick Hillegas added a comment - I don't have strong religion about what to call this attribute. I think that deregister=true is fine. I would prefer to leave it alone now that Kim is in the middle of documenting the feature.
          Hide
          Kim Haase added a comment -

          I might amend that to say, "Kim, who is already confused enough, is in the middle of documenting the feature"

          Show
          Kim Haase added a comment - I might amend that to say, "Kim, who is already confused enough, is in the middle of documenting the feature"
          Hide
          Lily Wei added a comment -

          Should we adjust the release note about deregister attribute so it is consistent with documentation?

          Show
          Lily Wei added a comment - Should we adjust the release note about deregister attribute so it is consistent with documentation?
          Hide
          Rick Hillegas added a comment -

          Attaching a new version of the release note for this issue. This version refers the reader to the Reference Manual for more information on the deregister attribute. Does this version look OK?

          Show
          Rick Hillegas added a comment - Attaching a new version of the release note for this issue. This version refers the reader to the Reference Manual for more information on the deregister attribute. Does this version look OK?
          Hide
          Lily Wei added a comment -

          Thanks Rick. The release note looks great. If there is anything I would add, it will be the actual link of deregister attribute in the Derby Reference Manual. However, I think it will be easy for users just Google it.

          Show
          Lily Wei added a comment - Thanks Rick. The release note looks great. If there is anything I would add, it will be the actual link of deregister attribute in the Derby Reference Manual. However, I think it will be easy for users just Google it.
          Hide
          Rick Hillegas added a comment -

          Hi Lily. We could add a link to the documentation for the deregister attribute, but the link won't work while we are evaluating the RC. That is because the 10.8 docs will not have been posted on the Derby site yet. I would prefer to not create that situation. Thanks.

          Show
          Rick Hillegas added a comment - Hi Lily. We could add a link to the documentation for the deregister attribute, but the link won't work while we are evaluating the RC. That is because the 10.8 docs will not have been posted on the Derby site yet. I would prefer to not create that situation. Thanks.
          Hide
          Lily Wei added a comment -

          Thanks Rick. This is like catch 22 problem. No, I wouldn't want to have that situation as well.

          Show
          Lily Wei added a comment - Thanks Rick. This is like catch 22 problem. No, I wouldn't want to have that situation as well.
          Hide
          Rick Hillegas added a comment -

          Attaching a new version of the release note. This version removes the blockquotes which violate the accessibility guidelines at http://www.w3.org/TR/WCAG10/

          Show
          Rick Hillegas added a comment - Attaching a new version of the release note. This version removes the blockquotes which violate the accessibility guidelines at http://www.w3.org/TR/WCAG10/
          Hide
          Sebastian added a comment - - edited

          I have the feeling this bug is not fixed or I have a similar problem.
          I use tomcat 6.0.26 and an embedded derby(Bundle-Name: Apache Derby 10.8 Bundle-Version: 10.8.2000002.1181258).
          There is always the following line logged into the catalina.log on stopping my tomcat.

          SEVERE: A web application registered the JBDC driver [org.apache.derby.jdbc.AutoloadedDriver40] but failed to unregister it when the web application was stopped. To prevent a memory leak, the JDBC Driver has been forcibly unregistered.

          Show
          Sebastian added a comment - - edited I have the feeling this bug is not fixed or I have a similar problem. I use tomcat 6.0.26 and an embedded derby(Bundle-Name: Apache Derby 10.8 Bundle-Version: 10.8.2000002.1181258). There is always the following line logged into the catalina.log on stopping my tomcat. SEVERE: A web application registered the JBDC driver [org.apache.derby.jdbc.AutoloadedDriver40] but failed to unregister it when the web application was stopped. To prevent a memory leak, the JDBC Driver has been forcibly unregistered.
          Hide
          Sebastian added a comment -

          Here my code for the shutdown:

          Context ctx = new InitialContext(env);
          EmbeddedConnectionPoolDataSource ds = (EmbeddedConnectionPoolDataSource)ctx.lookup(DB_CONTEXT);
          if (ds != null)

          { ds.setShutdownDatabase("shutdown"); ds.getConnection(); }
          Show
          Sebastian added a comment - Here my code for the shutdown: Context ctx = new InitialContext(env); EmbeddedConnectionPoolDataSource ds = (EmbeddedConnectionPoolDataSource)ctx.lookup(DB_CONTEXT); if (ds != null) { ds.setShutdownDatabase("shutdown"); ds.getConnection(); }
          Hide
          Knut Anders Hatlen added a comment -

          Maybe you're just shutting down the database, and not the database engine? To shut down the engine, make sure there's no database name set in the data source by calling ds.setDatabaseName(null) before getConnection().

          Show
          Knut Anders Hatlen added a comment - Maybe you're just shutting down the database, and not the database engine? To shut down the engine, make sure there's no database name set in the data source by calling ds.setDatabaseName(null) before getConnection().
          Hide
          Knut Anders Hatlen added a comment -

          [bulk update] Close all resolved issues that haven't been updated for more than one year.

          Show
          Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.

            People

            • Assignee:
              Lily Wei
              Reporter:
              Daniel John Debrunner
            • Votes:
              2 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development