Derby
  1. Derby
  2. DERBY-210

Network Server will leak prepared statements if not explicitly closed by the user until the connection is closed

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 10.0.2.0
    • Fix Version/s: None
    • Component/s: Network Client
    • Urgency:
      Normal

      Description

      Network server will not garbage collect prepared statements that are not explicitly closed by the user. So a loop like this will leak.

      ...
      PreparedStatement ps;

      for (int i = 0 ; i < numPs; i++)
      {

      ps = conn.prepareStatement(selTabSql);
      rs =ps.executeQuery();

      while (rs.next())

      { rs.getString(1); }

      rs.close();
      // I'm a sloppy java programmer
      //ps.close();

      }

      To reproduce run the attached program
      java derbyStress

      Both client and server will grow until the connection is closed.

      It is likely that the fix for this will have to be in the client. The client does not send protocol to close the prepared statement, but rather reuses the PKGNAMCSN on the PRPSQLSTT request once the prepared statement has been closed. This is how the server knows to close the old statement and create a new one.

      1. derbyStress.java
        12 kB
        Kathey Marsden
      2. DOTS_ATCJ2_Derby-noPatch.png
        6 kB
        John H. Embretsen
      3. DOTS_ATCJ2_Derby-withPatch.png
        7 kB
        John H. Embretsen
      4. derby-210-v2-draft.diff
        19 kB
        Deepa Remesh
      5. derby-210-v2-draft.status
        0.8 kB
        Deepa Remesh
      6. derby-210-patch1.diff
        2 kB
        Deepa Remesh
      7. derby-210-patch2.diff
        8 kB
        Deepa Remesh
      8. derby-210-patch2.status
        0.6 kB
        Deepa Remesh
      9. derby-210-patch3.diff
        0.8 kB
        Deepa Remesh
      10. derby-210-patch4-v2.diff
        6 kB
        Deepa Remesh
      11. derby-210-patch4-v2.status
        0.1 kB
        Deepa Remesh
      12. derby-210-patch4-v3.diff
        7 kB
        Deepa Remesh
      13. derby-210-patch4-v3.status
        0.1 kB
        Deepa Remesh
      14. derby-210-patch5-v1.diff
        10 kB
        Deepa Remesh
      15. derby-210-patch5-v1.status
        0.4 kB
        Deepa Remesh
      16. StatementStress.java
        17 kB
        John H. Embretsen
      17. runtimeinfo_DOTS-OOME.txt
        32 kB
        John H. Embretsen

        Issue Links

          Activity

          Hide
          Mike Matrigali added a comment -

          Triaged July 10, 2009: assigned normal urgency.

          Show
          Mike Matrigali added a comment - Triaged July 10, 2009: assigned normal urgency.
          Hide
          Deepa Remesh added a comment -

          I had been working on this issue and had unassigned myself after completing the work I intended to do (covered in sub-task DERBY-1103). I checked that there are no pending patches attached to DERBY-210. Hence unchecking the patch available check box.

          Show
          Deepa Remesh added a comment - I had been working on this issue and had unassigned myself after completing the work I intended to do (covered in sub-task DERBY-1103 ). I checked that there are no pending patches attached to DERBY-210 . Hence unchecking the patch available check box.
          Hide
          Deepa Remesh added a comment -

          I have opened a sub-task (DERBY-1103) for the work I have done for this issue for 10.2. I am unassigning myself from DERBY-210 as I will not be able to continue immediately with further work on it.

          Here is a list of other issues found when working on this one:

          • DERBY-1104
          • DERBY-1106
          • DERBY-1002
          • DERBY-817
          • John mentioned potential memory leaks seen in the DOTS run. As Kathey suggested, it would be good to open a new issue for this, with a smaller repro, if possible.
          Show
          Deepa Remesh added a comment - I have opened a sub-task ( DERBY-1103 ) for the work I have done for this issue for 10.2. I am unassigning myself from DERBY-210 as I will not be able to continue immediately with further work on it. Here is a list of other issues found when working on this one: DERBY-1104 DERBY-1106 DERBY-1002 DERBY-817 John mentioned potential memory leaks seen in the DOTS run. As Kathey suggested, it would be good to open a new issue for this, with a smaller repro, if possible.
          Hide
          Kathey Marsden added a comment -

          I committed Deepa's patch5
          Thank you so much Deepa for all your hard work on this issue and Bryan and John too.

          Date: Wed Mar 8 13:16:49 2006
          New Revision: 384331

          URL: http://svn.apache.org/viewcvs?rev=384331&view=rev

          I spoke with Deepa on IRC about the oustanding issues and possible Jira organization.

          The current oustanding issues seem to be:
          a) Prepared statements are not garbage collected on the server until the sections are reused. (This has to be fixed with a DRDA protocol Change)
          b) ResultSets are not garbage collected on the server until the section is reused. (This could be fixed with DERBY-1021)
          c) John found an issue with Statements not getting cleaned up at least in the server.

          Would this sound like a reasonable course of action?
          1) Open a subtask of this issue with the work Deepa has done so far and close it. Something like:
          "Change the client driver to allow statements to be garbage-collected once they are dereferenced by an application"
          2) Open a subtask of DERBY-210 for item a above.
          3) Open an issue for "b" and make DERBY-1021 part of it or just close out DERBY-1021
          4) Open a new issue for c

          Show
          Kathey Marsden added a comment - I committed Deepa's patch5 Thank you so much Deepa for all your hard work on this issue and Bryan and John too. Date: Wed Mar 8 13:16:49 2006 New Revision: 384331 URL: http://svn.apache.org/viewcvs?rev=384331&view=rev I spoke with Deepa on IRC about the oustanding issues and possible Jira organization. The current oustanding issues seem to be: a) Prepared statements are not garbage collected on the server until the sections are reused. (This has to be fixed with a DRDA protocol Change) b) ResultSets are not garbage collected on the server until the section is reused. (This could be fixed with DERBY-1021 ) c) John found an issue with Statements not getting cleaned up at least in the server. Would this sound like a reasonable course of action? 1) Open a subtask of this issue with the work Deepa has done so far and close it. Something like: "Change the client driver to allow statements to be garbage-collected once they are dereferenced by an application" 2) Open a subtask of DERBY-210 for item a above. 3) Open an issue for "b" and make DERBY-1021 part of it or just close out DERBY-1021 4) Open a new issue for c
          Hide
          Kathey Marsden added a comment -

          Committed 'derby-210-patch5-v1.diff'

          Date: Wed Mar 8 13:16:49 2006
          New Revision: 384331

          URL: http://svn.apache.org/viewcvs?rev=384331&view=rev

          Thank you so much Deepa for all your hard work on this issue and Bryan and John too.
          I talked with Deepa on IRC about the outstanding issues and possible organization of Jira issues, moving forward:

          The current oustanding issues seem to be:
          a) Prepared statements are not garbage collected on the server until the sections are reused. (This I think has to be fixed with a DRDA protocol Change)
          b) ResultSets are not garbage collected on the server until the section is reused. (This could be fixed with DERBY-1021)
          c) John found an issue with Statements not getting cleaned up at least on the server.

          Would this sound like a reasonable course of action?
          1) Open a subtask of this issue with the work Deepa has done so far and close it. Something like:
          "Change the client driver to allow statements to be garbage-collected once they are dereferenced by an application"
          2) Open a subtask of DERBY-210 for item "a" above.
          3) Open a new issue for "b" to replace DERBY-1021. DERBY-1021 could be a part of that issue or just closed.
          4) Open a new issue for "c"

          Thanks
          Kathey

          2)

          Would the following sound like a reasonable course of action moving forward:

          1) Close DERBY-1021 and replace it with a subtask of this issue that is more specific, like:

          Show
          Kathey Marsden added a comment - Committed 'derby-210-patch5-v1.diff' Date: Wed Mar 8 13:16:49 2006 New Revision: 384331 URL: http://svn.apache.org/viewcvs?rev=384331&view=rev Thank you so much Deepa for all your hard work on this issue and Bryan and John too. I talked with Deepa on IRC about the outstanding issues and possible organization of Jira issues, moving forward: The current oustanding issues seem to be: a) Prepared statements are not garbage collected on the server until the sections are reused. (This I think has to be fixed with a DRDA protocol Change) b) ResultSets are not garbage collected on the server until the section is reused. (This could be fixed with DERBY-1021 ) c) John found an issue with Statements not getting cleaned up at least on the server. Would this sound like a reasonable course of action? 1) Open a subtask of this issue with the work Deepa has done so far and close it. Something like: "Change the client driver to allow statements to be garbage-collected once they are dereferenced by an application" 2) Open a subtask of DERBY-210 for item "a" above. 3) Open a new issue for "b" to replace DERBY-1021 . DERBY-1021 could be a part of that issue or just closed. 4) Open a new issue for "c" Thanks Kathey 2) Would the following sound like a reasonable course of action moving forward: 1) Close DERBY-1021 and replace it with a subtask of this issue that is more specific, like:
          Hide
          John H. Embretsen added a comment -

          I attached runtimeinfo_DOTS-OOME.txt, which is runtimeinfo (org.apache.derby.drda.NetworkServerControl runtimeinfo) collected a while after the first OutOfMemoryError (OOME) on the server, from the last failed DOTS test run, as discussed in the e-mail thread
          http://www.nabble.com/-jira-Updated%3A-%28DERBY-210%29-Network-Server-will-leak-prepared-statements-if-not-explicitly-closed-by-the-user-until-the-connection-is-closed-t1219222.html#a3271800

          The first OOME occurred after roughly 56 hours this time. I have collected similar runtimeinfos from previous failed test runs.
          (I do not currently have a runtimeinfo from the exact time when the first OOME occurres.)

          Most of the statements seen in runtimeinfo are of the type

          SELECT SELLERID,DESCRIPTION,BID_PRICE,START_TIME,END_TIME,BID_COUNT FROM ITEM WHERE ITEMID = 'ITEM664222'

          which seem to be unprepared statements, e.g.

          rs = stmt.executeQuery(getItemSQL + "'" + itemID + "'");

          that are not always properly closed (see dots.advcase.ATCJ2.java).

          I guess that in this case the problem is a lack of closing all statements properly, regardless of whether they are prepared or not .

          Show
          John H. Embretsen added a comment - I attached runtimeinfo_DOTS-OOME.txt, which is runtimeinfo (org.apache.derby.drda.NetworkServerControl runtimeinfo) collected a while after the first OutOfMemoryError (OOME) on the server, from the last failed DOTS test run, as discussed in the e-mail thread http://www.nabble.com/-jira-Updated%3A-%28DERBY-210%29-Network-Server-will-leak-prepared-statements-if-not-explicitly-closed-by-the-user-until-the-connection-is-closed-t1219222.html#a3271800 The first OOME occurred after roughly 56 hours this time. I have collected similar runtimeinfos from previous failed test runs. (I do not currently have a runtimeinfo from the exact time when the first OOME occurres.) Most of the statements seen in runtimeinfo are of the type SELECT SELLERID,DESCRIPTION,BID_PRICE,START_TIME,END_TIME,BID_COUNT FROM ITEM WHERE ITEMID = 'ITEM664222' which seem to be unprepared statements, e.g. rs = stmt.executeQuery(getItemSQL + "'" + itemID + "'"); that are not always properly closed (see dots.advcase.ATCJ2.java). I guess that in this case the problem is a lack of closing all statements properly, regardless of whether they are prepared or not .
          Hide
          Kathey Marsden added a comment -

          I would like to suggest that we go ahead and commit Deepa's patch5, since it shows marked improvement and no signs of regression. Any objections? If I hear nothing I will checkin later this afternoon.

          John, it would be interesting to see the output of runtimeinfo at the 55 hour mark, to see if it is really prepared statements leaking or something else.

          Show
          Kathey Marsden added a comment - I would like to suggest that we go ahead and commit Deepa's patch5, since it shows marked improvement and no signs of regression. Any objections? If I hear nothing I will checkin later this afternoon. John, it would be interesting to see the output of runtimeinfo at the 55 hour mark, to see if it is really prepared statements leaking or something else.
          Hide
          John H. Embretsen added a comment -

          Uploaded my repro (StatementStress.java) for this issue.

          The repro is an independent Java program with elements inspired by some of the "naughtiest" parts of the infamous DOTS test case ATCJ2.java (see my previous comments to this issue).

          I have tried to incorporate lots of explicit comments in the code, so that I will not get bashed for not re-using or closing all PreparedStatements

          See main-method comments for usage instructions.

          In short, the program does the following:

          ====================================================================

          • Connects to a database, "naughtyWombat"
          • Creates a table with two columns:
          • ID CHAR(12) NOT NULL PRIMARY KEY
          • DESCR VARCHAR(50)
          • Inserts 10000 rows into this table (in a sane fashion, I hope)
          • The ID column consists of the text "ITEM-X", where X is a number between 1 and 10000.
          • The DESCR column is always "This is a dummy description"
          • Queries this table (by primary key) by:
          • Preparing a PreparedStatement (not using "?" placeholders, to stress the system)
          • Executing the query, and iterating through the result set
          • Closing the result set explicitly
          • Setting the PreparedStatement to null, without closing it explicitly first

          ====================================================================

          With Derby in Client/Server mode and a server JVM heap size of 128 MB and PermGen size of 64 MB (this is pretty close to the defaults on modern JVMs and desktop/server machines), I got the following results:

          • With Derby trunk at SVN 382319, with Sun JDK 1.5, the server is disabled by an OutOfMemoryError (PermGenSpace) after executing approximately 11000 query statements.
          • With Derby trunk + derby-210-patch5-v1.diff (Sun JDK1.5), I have executed more than 900000 queries without any errors (the JVM is stressed, but is able to GC generated classes).
          • I have also run the test against Derby in embedded mode, without seeing any errors.
          • The test did not pass with jdk1.4.2, but I have not had time to test much or investigate (I would appreciate if someone else could try this as well).

          There is one important thing to be aware of, though:

          It seems to me that it is not possible to eliminate this problem when the server is running on a 1.3 JVM!
          This is because classes are not unloaded from the permanent generation with 1.3.x JVMs (as far as I know).

          Feel free to experiment with this code and/or adding some version of it as a regression test, if appropriate...

          Show
          John H. Embretsen added a comment - Uploaded my repro (StatementStress.java) for this issue. The repro is an independent Java program with elements inspired by some of the "naughtiest" parts of the infamous DOTS test case ATCJ2.java (see my previous comments to this issue). I have tried to incorporate lots of explicit comments in the code, so that I will not get bashed for not re-using or closing all PreparedStatements See main-method comments for usage instructions. In short, the program does the following: ==================================================================== Connects to a database, "naughtyWombat" Creates a table with two columns: ID CHAR(12) NOT NULL PRIMARY KEY DESCR VARCHAR(50) Inserts 10000 rows into this table (in a sane fashion, I hope) The ID column consists of the text "ITEM-X", where X is a number between 1 and 10000. The DESCR column is always "This is a dummy description" Queries this table (by primary key) by: Preparing a PreparedStatement (not using "?" placeholders, to stress the system) Executing the query, and iterating through the result set Closing the result set explicitly Setting the PreparedStatement to null, without closing it explicitly first ==================================================================== With Derby in Client/Server mode and a server JVM heap size of 128 MB and PermGen size of 64 MB (this is pretty close to the defaults on modern JVMs and desktop/server machines), I got the following results: With Derby trunk at SVN 382319, with Sun JDK 1.5, the server is disabled by an OutOfMemoryError (PermGenSpace) after executing approximately 11000 query statements. With Derby trunk + derby-210-patch5-v1.diff (Sun JDK1.5), I have executed more than 900000 queries without any errors (the JVM is stressed, but is able to GC generated classes). I have also run the test against Derby in embedded mode, without seeing any errors. The test did not pass with jdk1.4.2, but I have not had time to test much or investigate (I would appreciate if someone else could try this as well). There is one important thing to be aware of, though: It seems to me that it is not possible to eliminate this problem when the server is running on a 1.3 JVM! This is because classes are not unloaded from the permanent generation with 1.3.x JVMs (as far as I know). Feel free to experiment with this code and/or adding some version of it as a regression test, if appropriate...
          Hide
          Deepa Remesh added a comment -

          Thanks John for the quick response. I really appreciate your testing effort and waiting to see the results.

          Meantime, I have run derbyall with 'derby-210-patch5-v1.diff' using Sun JDK 1.4.2 on Win XP. Only 1 failure (lang/wisconsin.java) also seen in nightlies.

          Show
          Deepa Remesh added a comment - Thanks John for the quick response. I really appreciate your testing effort and waiting to see the results. Meantime, I have run derbyall with 'derby-210-patch5-v1.diff' using Sun JDK 1.4.2 on Win XP. Only 1 failure (lang/wisconsin.java) also seen in nightlies.
          Hide
          John H. Embretsen added a comment -

          Regarding patch derby-210-patch5-v1.diff:

          I have not looked at the actual code, but I applied the patch to head of trunk and started to run some tests:

          • I started the previously mentioned DOTS test against trunk + patch, with same settings (or as close as possible) as earlier. If it runs without error for longer than 55 hours, it will be an improvement. I will publish the results later.
          • I created a smaller stand-alone application "inspired by" the really bad parts of the DOTS test, and got very promising results (using sane jars):
          • Without the patch, head of trunk (SVN 382319), OutOfMemoryError occured in the derby server JVM after executing a little more than 11000 "bad" SELECT statements.
          • With the patch applied, the the server JVM was able to garbage collect the PermSpace, and executed 100000 "bad" statements without error

          I will do some more testing, but it's certainly looking good so far!

          Show
          John H. Embretsen added a comment - Regarding patch derby-210-patch5-v1.diff: I have not looked at the actual code, but I applied the patch to head of trunk and started to run some tests: I started the previously mentioned DOTS test against trunk + patch, with same settings (or as close as possible) as earlier. If it runs without error for longer than 55 hours, it will be an improvement. I will publish the results later. I created a smaller stand-alone application "inspired by" the really bad parts of the DOTS test, and got very promising results (using sane jars): Without the patch, head of trunk (SVN 382319), OutOfMemoryError occured in the derby server JVM after executing a little more than 11000 "bad" SELECT statements. With the patch applied, the the server JVM was able to garbage collect the PermSpace, and executed 100000 "bad" statements without error I will do some more testing, but it's certainly looking good so far!
          Hide
          Deepa Remesh added a comment -

          Attaching a patch 'derby-210-patch5-v1.diff' which removes the actual memory leaks. The previous patches were doing preliminary work to fix the client-side finalization and network server object re-use. This patch removes the actual memory leaks.

          -----------------------------------------------------------------
          Summary of patch:
          -----------------------------------------------------------------
          1. Eliminates the below references to PreparedStatement objects by using WeakHashMap instead of LinkedList. When there are no other references to the keys in a WeakHashMap, they will get removed from the map and can thus get garbage-collected. They do not have to wait till the Connection object is collected.

          • 'openStatements_' in org.apache.derby.client.am.Connection
          • 'CommitAndRollbackListeners_' in org.apache.derby.client.am.Connection

          2. Updates the following comment for openStatements_:
          // Since DERBY prepared statements must be re-prepared after a commit,
          // then we must traverse this list after a commit and notify statements
          // that they are now in an un-prepared state.
          final java.util.LinkedList openStatements_ = new java.util.LinkedList();

          In the code, I did not see this list being traversed after a commit to re-prepare statements. Also, I think this is not needed since Derby does not require re-prepare of statements after a commit. Currently, this list is used to close all open statements when the originating connection is closed.

          3. Removes all ResultSets from HashTable 'positionedUpdateCursorNameToResultSet_' in SectionManager. Only result sets of positioned update statements were being removed from this hashtable whereas all result sets were added. Because of this, client driver was holding on to result sets and statements even after rs.close() was called.

          4. Modifies the test jdbcapi/derbyStress.java to run the test for derby-210. The test was checked in as patch2 but disabled for client framework.

          -----------------------------------------------------------------
          Open items related to this issue
          -----------------------------------------------------------------

          • http://issues.apache.org/jira/browse/DERBY-817 (Improvements to Network Client driver - analyze/improve use of java collection classes in the code) - This issue lists some potential problem areas and cleanup tasks identified in the client driver code.

          With this patch, I ran the following tests on Windows XP:

          • derbynetclientmats suite 6 times on my machine (3 times with Sun JDK1.4.2 and 3 times with Sun JDK1.5). No failures.
          • repro with 100,000 statements and checked the memory usage on client and server. No spikes in memory usage. Without this patch, the repro used to give OutOfMemory error after executing 950 statements. Tests run with Sun JDK1.4.2 with 64M heap space .
          • test to compare memory usage when statements_are_not_explicitly_closed vs statements_are_explicitly_closed. Tests run with Sun JDK1.4.2 with 64M heap space.
            case1: repro with 5000 statements
            case2: modified_repro_which_explicitly_closes_prepared_statements with 5000 statements.
            I compared the memory usage on client and server in these two cases and saw that memory usage over time is similar. Memory usage is calculated as Runtime.totalMemory() - Runtime.freeMemory().

          I would appreciate if someone can take a look at this patch.

          Show
          Deepa Remesh added a comment - Attaching a patch 'derby-210-patch5-v1.diff' which removes the actual memory leaks. The previous patches were doing preliminary work to fix the client-side finalization and network server object re-use. This patch removes the actual memory leaks. ----------------------------------------------------------------- Summary of patch: ----------------------------------------------------------------- 1. Eliminates the below references to PreparedStatement objects by using WeakHashMap instead of LinkedList. When there are no other references to the keys in a WeakHashMap, they will get removed from the map and can thus get garbage-collected. They do not have to wait till the Connection object is collected. 'openStatements_' in org.apache.derby.client.am.Connection 'CommitAndRollbackListeners_' in org.apache.derby.client.am.Connection 2. Updates the following comment for openStatements_: // Since DERBY prepared statements must be re-prepared after a commit, // then we must traverse this list after a commit and notify statements // that they are now in an un-prepared state. final java.util.LinkedList openStatements_ = new java.util.LinkedList(); In the code, I did not see this list being traversed after a commit to re-prepare statements. Also, I think this is not needed since Derby does not require re-prepare of statements after a commit. Currently, this list is used to close all open statements when the originating connection is closed. 3. Removes all ResultSets from HashTable 'positionedUpdateCursorNameToResultSet_' in SectionManager. Only result sets of positioned update statements were being removed from this hashtable whereas all result sets were added. Because of this, client driver was holding on to result sets and statements even after rs.close() was called. 4. Modifies the test jdbcapi/derbyStress.java to run the test for derby-210. The test was checked in as patch2 but disabled for client framework. ----------------------------------------------------------------- Open items related to this issue ----------------------------------------------------------------- http://issues.apache.org/jira/browse/DERBY-817 (Improvements to Network Client driver - analyze/improve use of java collection classes in the code) - This issue lists some potential problem areas and cleanup tasks identified in the client driver code. http://issues.apache.org/jira/browse/DERBY-1002 (Check that DRDAStatement and DRDAResultSet states are reset when they are re-used) - This issue was blocking DERBY-210 . Patch for the blocking issue has been committed. There is still some minor cleanup to be done in this issue. http://issues.apache.org/jira/browse/DERBY-1021 (Perform cleanup actions which require synchronization outside the finalizer) - This issue is linked to ("is part of") DERBY-210 . Maybe, it is easier if I had made this a sub-task of DERBY-210 . I will close DERBY-1021 and open a sub-task for 210 with details about work needed on the finalizer. With this patch, I ran the following tests on Windows XP: derbynetclientmats suite 6 times on my machine (3 times with Sun JDK1.4.2 and 3 times with Sun JDK1.5). No failures. repro with 100,000 statements and checked the memory usage on client and server. No spikes in memory usage. Without this patch, the repro used to give OutOfMemory error after executing 950 statements. Tests run with Sun JDK1.4.2 with 64M heap space . test to compare memory usage when statements_are_not_explicitly_closed vs statements_are_explicitly_closed. Tests run with Sun JDK1.4.2 with 64M heap space. case1: repro with 5000 statements case2: modified_repro_which_explicitly_closes_prepared_statements with 5000 statements. I compared the memory usage on client and server in these two cases and saw that memory usage over time is similar. Memory usage is calculated as Runtime.totalMemory() - Runtime.freeMemory(). I would appreciate if someone can take a look at this patch.
          Hide
          Kathey Marsden added a comment -

          Committed 'derby-210-patch4-v3.diff'
          Date: Fri Feb 24 19:57:51 2006
          New Revision: 380892

          URL: http://svn.apache.org/viewcvs?rev=380892&view=rev

          Show
          Kathey Marsden added a comment - Committed 'derby-210-patch4-v3.diff' Date: Fri Feb 24 19:57:51 2006 New Revision: 380892 URL: http://svn.apache.org/viewcvs?rev=380892&view=rev
          Hide
          Kathey Marsden added a comment -

          Thanks Deepa. The new comments look good. I will kick of derbynetclientmats and commit later

          Show
          Kathey Marsden added a comment - Thanks Deepa. The new comments look good. I will kick of derbynetclientmats and commit later
          Hide
          Deepa Remesh added a comment -

          Attaching a patch 'derby-210-patch4-v3.diff'. In this patch, I synched upto latest revision and added the additional comments suggested by Kathey. There are no other changes from previous version (derby-210-patch4-v2.diff). With v3 patch, I have run derbynetclientmats with SunJDK 1.4.2 on WinXP.

          I have opened two other JIRA issues related to DERBY-210:

          I would appreciate if someone can review/commit patch4.

          Show
          Deepa Remesh added a comment - Attaching a patch 'derby-210-patch4-v3.diff'. In this patch, I synched upto latest revision and added the additional comments suggested by Kathey. There are no other changes from previous version (derby-210-patch4-v2.diff). With v3 patch, I have run derbynetclientmats with SunJDK 1.4.2 on WinXP. I have opened two other JIRA issues related to DERBY-210 : DERBY-1021 ( http://issues.apache.org/jira/browse/DERBY-1021 ) - This is for work on finalizers and can be worked on independently. DERBY-1002 ( http://issues.apache.org/jira/browse/DERBY-1021 ) - This is for cleaning up the re-use code in network server. This issue blocks my planned patch5 for DERBY-210 . I would appreciate if someone can review/commit patch4.
          Hide
          Kathey Marsden added a comment -

          There was a separate thread on this issue on the list where concerns were voiced about doing the result set cleanup work in finalize and maybe creating another thread to do that.

          I talked with Deepa a bit on IRC about the impact of submitting her patch as is without the result set cleanup.
          The summary is.

          Before Deepa's patch 4:

          • No statements or result sets get cleaned up until the end of the connection unless explicitly closed

          After Deepa's patch 4:

          • No statements or result sets get cleaned up until the end of the connection unless explicitly closed.

          After Deepa's planned patch 5:
          Most statements and result sets get cleaned up automatically.

          So, I am of the opinion that Deepa's patch can go in as is and another Jira entry filed for the result set cleanup. Her planned work is a huge improvement over the current state. She does not need to include the result set cleanup in her patch for DERBY-210

          Show
          Kathey Marsden added a comment - There was a separate thread on this issue on the list where concerns were voiced about doing the result set cleanup work in finalize and maybe creating another thread to do that. I talked with Deepa a bit on IRC about the impact of submitting her patch as is without the result set cleanup. The summary is. Before Deepa's patch 4: No statements or result sets get cleaned up until the end of the connection unless explicitly closed After Deepa's patch 4: No statements or result sets get cleaned up until the end of the connection unless explicitly closed. After Deepa's planned patch 5: Most statements and result sets get cleaned up automatically. So, I am of the opinion that Deepa's patch can go in as is and another Jira entry filed for the result set cleanup. Her planned work is a huge improvement over the current state. She does not need to include the result set cleanup in her patch for DERBY-210
          Hide
          Deepa Remesh added a comment -

          Thanks Kathey for the test case. I got what you are trying to say and I am working on making the changes. However, I could not verify your test case with the current client code (I could verify it with few changes just for testing). With current client code, if we don't close a result set explicitly, it's statement object will not be freed. This is because result set gets added to 'positionedUpdateCursorNameToResultSet_' table in SectionManager. The result set object gets removed from this table only when ResultSet.markClosed() gets called. So the test you suggested cannot check the status of objects on network server since the current client-side cleanup itself is not complete. This issue is listed in DERBY-817.

          Show
          Deepa Remesh added a comment - Thanks Kathey for the test case. I got what you are trying to say and I am working on making the changes. However, I could not verify your test case with the current client code (I could verify it with few changes just for testing). With current client code, if we don't close a result set explicitly, it's statement object will not be freed. This is because result set gets added to 'positionedUpdateCursorNameToResultSet_' table in SectionManager. The result set object gets removed from this table only when ResultSet.markClosed() gets called. So the test you suggested cannot check the status of objects on network server since the current client-side cleanup itself is not complete. This issue is listed in DERBY-817 .
          Hide
          Kathey Marsden added a comment -

          Thanks Deepa.

          Perhaps this test case would show if the ResultSets are being closed.

          • create a table and insert a few rows with more than 32K of data.
          • create a prepared statement to drop the table.
          • fetch 1 row
          • dereference the statement
          • force garbage collection
          • try to drop the table with the prepared statement.

          I t hink if the ResultSet is not closed on garbage collection, you will not be able to drop the table.
          I have not tried this, but just a thought.

          Show
          Kathey Marsden added a comment - Thanks Deepa. Perhaps this test case would show if the ResultSets are being closed. create a table and insert a few rows with more than 32K of data. create a prepared statement to drop the table. fetch 1 row dereference the statement force garbage collection try to drop the table with the prepared statement. I t hink if the ResultSet is not closed on garbage collection, you will not be able to drop the table. I have not tried this, but just a thought.
          Hide
          Deepa Remesh added a comment -

          Thanks Kathey.

          In short, this is what I have been trying to do in patch4 - Limit the finalize in client statement classes to client side cleanup and let the server handle the cleanup of its objects based on current logic of statement re-use. So I changed finalize to not send any network commands.

          But this will not work well for Kathey's scenario, where an application keeps all its statement references and dereferences all of them in the end. With my change in finalize, the result sets will get cleaned up only when client driver re-uses a section number for a new statement which will make network server close it's statement and free up result sets OR when the connection is closed.

          On first look, Kathey's scenario seemed highly improbable but I guess an application can do that if it stores references to statements in some hashtable. I will try to create a repro for Kathey's scenario and add this case to derbyStress.java and see the behaviour with my patches.

          I'm reworking finalize method to continue sending CLSQRY as Kathey suggested and will upload new version of patch 4.

          Show
          Deepa Remesh added a comment - Thanks Kathey. In short, this is what I have been trying to do in patch4 - Limit the finalize in client statement classes to client side cleanup and let the server handle the cleanup of its objects based on current logic of statement re-use. So I changed finalize to not send any network commands. But this will not work well for Kathey's scenario, where an application keeps all its statement references and dereferences all of them in the end. With my change in finalize, the result sets will get cleaned up only when client driver re-uses a section number for a new statement which will make network server close it's statement and free up result sets OR when the connection is closed. On first look, Kathey's scenario seemed highly improbable but I guess an application can do that if it stores references to statements in some hashtable. I will try to create a repro for Kathey's scenario and add this case to derbyStress.java and see the behaviour with my patches. I'm reworking finalize method to continue sending CLSQRY as Kathey suggested and will upload new version of patch 4.
          Hide
          Kathey Marsden added a comment -

          I am rephrasing this comment. I mispoke about the risidual statements not getting cleaned up. That is and will remain a problem because there is no CLSSTMT in DRDA. But this is what I meant to say about the result sets

          I think there actually is a problem with not sending CLSQRY for finalize. If I have 20,000 open valid and referenced prepared statements and then I dereference them. Their result sets will not get cleaned up until the connection ends because no CLSQRY will be sent.

          The reason for the change was to take the autocommit out of finalize and with that I fully agree, but I wonder: why do we have to take out the CLSQRY? writeCloseResultSets takes a boolean allowAutocommits. Could we continue to call it for finalize but just with that parameter set to false?

          Also I would appreciate input from others on this.

          Show
          Kathey Marsden added a comment - I am rephrasing this comment. I mispoke about the risidual statements not getting cleaned up. That is and will remain a problem because there is no CLSSTMT in DRDA. But this is what I meant to say about the result sets I think there actually is a problem with not sending CLSQRY for finalize. If I have 20,000 open valid and referenced prepared statements and then I dereference them. Their result sets will not get cleaned up until the connection ends because no CLSQRY will be sent. The reason for the change was to take the autocommit out of finalize and with that I fully agree, but I wonder: why do we have to take out the CLSQRY? writeCloseResultSets takes a boolean allowAutocommits. Could we continue to call it for finalize but just with that parameter set to false? Also I would appreciate input from others on this.
          Hide
          Deepa Remesh added a comment -

          Kathey, please see if the following answers your questions:

          2) Not sure about these. How do they get cleaned up?
          The statement and result sets for generated keys query get cleaned up in Statement.markClosed() by call to markPreparedStatementForAutoGeneratedKeysClosed() method. This method calls markClosed on the auto-generated keys statement, which will ensure this statement and its result sets are cleaned on client side. The cleanup on server happens when section is re-used.

          1) Somehow all of this causes the section to be reused. So, while we won't have immediate cleanup on the server, it will happen if the section is reused. Can you explain this process a bit more?
          As part of Statement.markClosed(), markClosedOnServer() method gets called. This method frees up the Section on the client. A stack is used to store the free sections. Hence, client driver will use this freed section for the next statement. When client driver re-uses the freed section, network server will find a statement with this section in its stmtTable and re-use the statement. This happens in Database.newDrdaStatement method in network server. Before re-using the statement, its close method is called. This close method cleans up the statement state and its current result set. All other resultsets in its result set table are freed.

          If you find these explanations are correct, I'll upload a revised version of patch4 with more comments.

          As you said, the code in patch4 will be not be fully covered by tests currently. The code in finalize method will be covered only after the patch which removes the memory leaks is reviewed and committed. This is in my next patch (patch5 - which removes the memory leaks + enable derbyStress test). I will run tests with patch4 + patch 5 and submit patch5, if that will help testing/reviewing.

          I think it would be good to do a long running test with patch4+patch5 before committing them. I'll be running the repro derbyStress.java with 1M prepared statements and will check client and server memory usage. Other than that, it may be good to run the patch through DOTS test. I do not have the DOTS setup. I would appreciate if John or anyone else has the setup for DOTS and will be willing to run the tests after I upload the new patches. Anyone has suggestions for other tests I can run to test this?

          Show
          Deepa Remesh added a comment - Kathey, please see if the following answers your questions: 2) Not sure about these. How do they get cleaned up? The statement and result sets for generated keys query get cleaned up in Statement.markClosed() by call to markPreparedStatementForAutoGeneratedKeysClosed() method. This method calls markClosed on the auto-generated keys statement, which will ensure this statement and its result sets are cleaned on client side. The cleanup on server happens when section is re-used. 1) Somehow all of this causes the section to be reused. So, while we won't have immediate cleanup on the server, it will happen if the section is reused. Can you explain this process a bit more? As part of Statement.markClosed(), markClosedOnServer() method gets called. This method frees up the Section on the client. A stack is used to store the free sections. Hence, client driver will use this freed section for the next statement. When client driver re-uses the freed section, network server will find a statement with this section in its stmtTable and re-use the statement. This happens in Database.newDrdaStatement method in network server. Before re-using the statement, its close method is called. This close method cleans up the statement state and its current result set. All other resultsets in its result set table are freed. If you find these explanations are correct, I'll upload a revised version of patch4 with more comments. As you said, the code in patch4 will be not be fully covered by tests currently. The code in finalize method will be covered only after the patch which removes the memory leaks is reviewed and committed. This is in my next patch (patch5 - which removes the memory leaks + enable derbyStress test). I will run tests with patch4 + patch 5 and submit patch5, if that will help testing/reviewing. I think it would be good to do a long running test with patch4+patch5 before committing them. I'll be running the repro derbyStress.java with 1M prepared statements and will check client and server memory usage. Other than that, it may be good to run the patch through DOTS test. I do not have the DOTS setup. I would appreciate if John or anyone else has the setup for DOTS and will be willing to run the tests after I upload the new patches. Anyone has suggestions for other tests I can run to test this?
          Hide
          Kathey Marsden added a comment -

          Statement.finalize() comment says

          • This method cleans up client-side resources by calling markClosed().
          • It is different from close() method, which also does clean up on server.

          and the comment for closeX() says
          /**

          • An untraced version of close(). This method cleans up client-side
          • resources and also sends commands to network server to perform
          • clean up. This should not be called in the finalizer.
          • @throws SqlException

          I think that the closeX comment should be expanded a bit to talk about the other differences between finalize() and closeX

          Looking at writeCloseResultSets, where all the action is, it seems the difference between finalize() and closeX() is closeX:
          1) Sends commands to the server to close the result sets.
          2) Sends commands to the server to close the result sets of the generated keys query.
          3) Sends a commit if autocommit is on and it is appropriate.
          4) Explicitly removes the statement from connection_.openStatements_ and CommitAndRollbackListeners_ by passing true to markClosed.

          Working backwards for finalizer(), as best I can tell, the finalizer does not need to do these things because ...
          4) The fact that it is being garbage collected means it is not in connection_.openStatements or CommitAndRollbackListners.
          3) It is not a good idea for the gc() to be sending commits.
          2) Not sure about these. How do they get cleaned up?
          1) Somehow all of this causes the section to be reused. So, while we won't have immediate cleanup on the server, it will happen if the section is reused. Can you explain this process a bit more?

          I think these changes are good, but I do think the comments regarding the difference between finalize() and closeX() need to be clearer. At first I thought adding a parameter to markClosed for removeListners was confusing, but I think it is ok since we hopefully will be able to get rid of these lists all together long term.

          Thanks for your help understanding these changes. I want to understand them well because we are not currently able to test them until your next patch goes in, and problems with gc() are always a bear to debug.

          Show
          Kathey Marsden added a comment - Statement.finalize() comment says This method cleans up client-side resources by calling markClosed(). It is different from close() method, which also does clean up on server. and the comment for closeX() says /** An untraced version of close(). This method cleans up client-side resources and also sends commands to network server to perform clean up. This should not be called in the finalizer. @throws SqlException I think that the closeX comment should be expanded a bit to talk about the other differences between finalize() and closeX Looking at writeCloseResultSets, where all the action is, it seems the difference between finalize() and closeX() is closeX: 1) Sends commands to the server to close the result sets. 2) Sends commands to the server to close the result sets of the generated keys query. 3) Sends a commit if autocommit is on and it is appropriate. 4) Explicitly removes the statement from connection_.openStatements_ and CommitAndRollbackListeners_ by passing true to markClosed. Working backwards for finalizer(), as best I can tell, the finalizer does not need to do these things because ... 4) The fact that it is being garbage collected means it is not in connection_.openStatements or CommitAndRollbackListners. 3) It is not a good idea for the gc() to be sending commits. 2) Not sure about these. How do they get cleaned up? 1) Somehow all of this causes the section to be reused. So, while we won't have immediate cleanup on the server, it will happen if the section is reused. Can you explain this process a bit more? I think these changes are good, but I do think the comments regarding the difference between finalize() and closeX() need to be clearer. At first I thought adding a parameter to markClosed for removeListners was confusing, but I think it is ok since we hopefully will be able to get rid of these lists all together long term. Thanks for your help understanding these changes. I want to understand them well because we are not currently able to test them until your next patch goes in, and problems with gc() are always a bear to debug.
          Hide
          Deepa Remesh added a comment -

          Thanks Kathey for reading patch4.

          The purpose of these lists do not seem to be same as what is indicated in the code comments. In my work for DERBY-210, I found objects added to these lists is one of the causes of memory leak. Solution that was suggested by you and Bernt was to use weak references in these lists. In my patch proposal, I have mentioned use of WeakHashMap instead of LinkedList.

          I tried to see if I can remove these lists but thought it to be too disruptive since the usage of these lists is not very clear. So I have opened DERBY-817 for further work on these lists. Please check DERBY-817 to see if it answers some of your questions.

          I am trying to summarize my understanding about these lists:
          -------------------------------------------------
          openStatements_ :
          -------------------------------------------------

          • What is it used for?
            When Connection.close() is called, this list is checked to get a list of open statements and statements are marked closed and removed from the list. This happens in Connection.markStatementsClosed() method. They also are used in reset method called by ClientPooledConnection to reset statements when re-using the same physical connection.
          • What is added and when?
            All statement objects, except positioned update statements, are added to this list. This happens in createStatement and prepare* methods in Connection class. For positioned update statements, there is a comment in the code in Connection.preparePositionedUpdateStatement:
            ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
            // The positioned update statement is not added to the list of open statements,
            // because this would cause a java.util.ConcurrentModificationException when
            // iterating thru the list of open statements to call completeRollback().
            // An updatable result set is marked closed on a call to completeRollback(),
            // and would therefore need to close the positioned update statement associated with the result set which would cause
            // it to be removed from the open statements list. Resulting in concurrent modification
            // on the open statements list.
            // Notice that ordinary Statement.closeX() is never called on the positioned update statement,
            // rather markClosed() is called to avoid trying to remove the statement from the openStatements_ list.
            ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
          • What is removed and when (finalize/close/some other time) ?
            Currently, removal happens when 1) Connection is closed 2) Statement is (explicitly) closed. 3) Code to remove_from_list is also there is the finalize method call path. But this is a no-op since finalize() will get called only when there are no references to objects in any lists.

          -------------------------------------------------
          CommitAndRollbackListeners_ :
          -------------------------------------------------

          • What is it used for?
            Please see DERBY-817. It seems like this list is not needed at least for purposes indicated by comment. This may need further investigation.
          • What is added and when?
            PreparedStatements, ResultSets and Lobs are added to this list from their listenToUnitOfWork() methods.
          • What is removed and when (finalize/close/some other time) ?
            For all three types of objects, removal happens in their completeLocalCommit and completeLocalRollback methods. For PreparedStatement and ResultSet, they are also removed from this list when they are closed.

          ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
          What patch4 (it changes finalization of statement objects) does to removal from these lists :
          ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
          Removal of statement objects from these two lists is moved to markClosed method. With respect to these lists, whatever was previosuly done by markClosed() method remains unchanged. A new method markClosed(boolean removeListener) is added to distinguish when the objects will be removed from the list. markClosed(true) is only called from close() methods. In this case, there is no change from previous behaviour since the following was previously done in close() method:
          ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
          markClosed();
          connection_.openStatements_.remove(this);
          ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
          and now it is replaced with
          ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
          markClosed(true);
          ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

          Before this patch, finalize method code path also had remove_from_list call. But this would never be visited. For finalize to be called, there should not be any reference to the statement object from any lists. Applying the same logic, in the patch, finalize just calls markClosed(), which does not have remove_from_list call.

          I hope this helps you in continuing to read the patch.

          Show
          Deepa Remesh added a comment - Thanks Kathey for reading patch4. The purpose of these lists do not seem to be same as what is indicated in the code comments. In my work for DERBY-210 , I found objects added to these lists is one of the causes of memory leak. Solution that was suggested by you and Bernt was to use weak references in these lists. In my patch proposal, I have mentioned use of WeakHashMap instead of LinkedList. I tried to see if I can remove these lists but thought it to be too disruptive since the usage of these lists is not very clear. So I have opened DERBY-817 for further work on these lists. Please check DERBY-817 to see if it answers some of your questions. I am trying to summarize my understanding about these lists: ------------------------------------------------- openStatements_ : ------------------------------------------------- What is it used for? When Connection.close() is called, this list is checked to get a list of open statements and statements are marked closed and removed from the list. This happens in Connection.markStatementsClosed() method. They also are used in reset method called by ClientPooledConnection to reset statements when re-using the same physical connection. What is added and when? All statement objects, except positioned update statements, are added to this list. This happens in createStatement and prepare* methods in Connection class. For positioned update statements, there is a comment in the code in Connection.preparePositionedUpdateStatement: ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ // The positioned update statement is not added to the list of open statements, // because this would cause a java.util.ConcurrentModificationException when // iterating thru the list of open statements to call completeRollback(). // An updatable result set is marked closed on a call to completeRollback(), // and would therefore need to close the positioned update statement associated with the result set which would cause // it to be removed from the open statements list. Resulting in concurrent modification // on the open statements list. // Notice that ordinary Statement.closeX() is never called on the positioned update statement, // rather markClosed() is called to avoid trying to remove the statement from the openStatements_ list. ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ What is removed and when (finalize/close/some other time) ? Currently, removal happens when 1) Connection is closed 2) Statement is (explicitly) closed. 3) Code to remove_from_list is also there is the finalize method call path. But this is a no-op since finalize() will get called only when there are no references to objects in any lists. ------------------------------------------------- CommitAndRollbackListeners_ : ------------------------------------------------- What is it used for? Please see DERBY-817 . It seems like this list is not needed at least for purposes indicated by comment. This may need further investigation. What is added and when? PreparedStatements, ResultSets and Lobs are added to this list from their listenToUnitOfWork() methods. What is removed and when (finalize/close/some other time) ? For all three types of objects, removal happens in their completeLocalCommit and completeLocalRollback methods. For PreparedStatement and ResultSet, they are also removed from this list when they are closed. ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- What patch4 (it changes finalization of statement objects) does to removal from these lists : ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Removal of statement objects from these two lists is moved to markClosed method. With respect to these lists, whatever was previosuly done by markClosed() method remains unchanged. A new method markClosed(boolean removeListener) is added to distinguish when the objects will be removed from the list. markClosed(true) is only called from close() methods. In this case, there is no change from previous behaviour since the following was previously done in close() method: ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ markClosed(); connection_.openStatements_.remove(this); ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ and now it is replaced with ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ markClosed(true); ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ Before this patch, finalize method code path also had remove_from_list call. But this would never be visited. For finalize to be called, there should not be any reference to the statement object from any lists. Applying the same logic, in the patch, finalize just calls markClosed(), which does not have remove_from_list call. I hope this helps you in continuing to read the patch.
          Hide
          Kathey Marsden added a comment -

          Firstly Deepa, let me say I am sorry I did not get to look at this part of the big patch when it was first posted.
          I think I need a little more background on the two lists in questions before I can review.
          Connection.openStatements_ and Connection.CommitAndRollbackListeners_

          Why are they needed?

          If they are needed....
          What are they used for?
          What is added and when?
          What is removed and when (finalize/close/some other time)

          The descriptions of both lists seem to describe things that I question.
          The statements do not need to be reprepared on the server after commit/rollback.
          I also don't understand why the resultset would be unpositioned.

          //DERBY prepared statements must be re-prepared after a commit
          // then we must traverse this list after a commit and notify statements
          // that they are now in an un-prepared state.
          final java.util.LinkedList openStatements_ = new java.util.LinkedList();

          // Some statuses of DERBY objects may be invalid on server
          // after both commit and rollback. For example,
          // (1) prepared statements need to be re-prepared
          // after both commit and rollback
          // (2) result set will be unpositioned on server after both commit and rollback.
          // If they depend on both commit and rollback, they need to get on CommitAndRollbackListeners_.
          final java.util.LinkedList CommitAndRollbackListeners_ = new java.util.LinkedList();

          Show
          Kathey Marsden added a comment - Firstly Deepa, let me say I am sorry I did not get to look at this part of the big patch when it was first posted. I think I need a little more background on the two lists in questions before I can review. Connection.openStatements_ and Connection.CommitAndRollbackListeners_ Why are they needed? If they are needed.... What are they used for? What is added and when? What is removed and when (finalize/close/some other time) The descriptions of both lists seem to describe things that I question. The statements do not need to be reprepared on the server after commit/rollback. I also don't understand why the resultset would be unpositioned. //DERBY prepared statements must be re-prepared after a commit // then we must traverse this list after a commit and notify statements // that they are now in an un-prepared state. final java.util.LinkedList openStatements_ = new java.util.LinkedList(); // Some statuses of DERBY objects may be invalid on server // after both commit and rollback. For example, // (1) prepared statements need to be re-prepared // after both commit and rollback // (2) result set will be unpositioned on server after both commit and rollback. // If they depend on both commit and rollback, they need to get on CommitAndRollbackListeners_. final java.util.LinkedList CommitAndRollbackListeners_ = new java.util.LinkedList();
          Hide
          Deepa Remesh added a comment -

          'derby-210-patch4-v2.diff' is available for review. Please take a look at this patch.

          Status of attached patches:

          • Patches already committed: derby-210-patch1.diff, derby-210-patch2.diff
          • Patches which should not be committed: derby-210-v2-draft.diff, derby-210-patch3.diff (problem solved in derby-614)
          • Patch available for review: derby-210-patch4-v2.diff
          Show
          Deepa Remesh added a comment - 'derby-210-patch4-v2.diff' is available for review. Please take a look at this patch. Status of attached patches: Patches already committed: derby-210-patch1.diff, derby-210-patch2.diff Patches which should not be committed: derby-210-v2-draft.diff, derby-210-patch3.diff (problem solved in derby-614) Patch available for review: derby-210-patch4-v2.diff
          Hide
          Deepa Remesh added a comment -

          Attaching a patch 'derby-210-patch4-v2.diff' which fixes the finalization code in Statement classes in client driver. This patch is in preparation for the fixes to memory leaks.

          Summary of changes:

          • Changes the finalizers in Statement classes to do only client-side cleanup. No network operations are done in the finalizers.
          • Adds markClosed method to PreparedStatement class which overrides the markClosed method in Statement class. All client-side cleanup code is moved to markClosed method. Two forms of markClosed method are added:
          • markClosed() method will perform clean up but will not remove Statement from the open statements list and PreparedStatement from the commit and rollback listeners list in org.apache.derby.client.am.Connection. This method is called from finalize(), Connection#markStatementsClosed(), and to close positioned update statements. For the finaizer to be called, the Statement should not have any references and so it should have been already removed from the lists. Connection#markStatementsClosed method explicitly removes the Statement from open statements list. Positioned update statements are not added to the open statements list. Hence, markClosed() is called in these cases.
          • markClosed(true) will additionally remove the statement objects from the lists. This is called from close() method.
          • After these changes, it is not required to override close(), closeX() and finalize() methods in PreparedStatement class since they contain same code as in Statement class. So these methods are removed.

          With these changes, I ran derbynetclientmats with Sun JDK 1.4.2. jdbcapi/dbMetaDataJdbc30.java failed. This test has been already fixed by David.

          Please take a look at this patch. Thanks.

          Show
          Deepa Remesh added a comment - Attaching a patch 'derby-210-patch4-v2.diff' which fixes the finalization code in Statement classes in client driver. This patch is in preparation for the fixes to memory leaks. Summary of changes: Changes the finalizers in Statement classes to do only client-side cleanup. No network operations are done in the finalizers. Adds markClosed method to PreparedStatement class which overrides the markClosed method in Statement class. All client-side cleanup code is moved to markClosed method. Two forms of markClosed method are added: markClosed() method will perform clean up but will not remove Statement from the open statements list and PreparedStatement from the commit and rollback listeners list in org.apache.derby.client.am.Connection. This method is called from finalize(), Connection#markStatementsClosed(), and to close positioned update statements. For the finaizer to be called, the Statement should not have any references and so it should have been already removed from the lists. Connection#markStatementsClosed method explicitly removes the Statement from open statements list. Positioned update statements are not added to the open statements list. Hence, markClosed() is called in these cases. markClosed(true) will additionally remove the statement objects from the lists. This is called from close() method. After these changes, it is not required to override close(), closeX() and finalize() methods in PreparedStatement class since they contain same code as in Statement class. So these methods are removed. With these changes, I ran derbynetclientmats with Sun JDK 1.4.2. jdbcapi/dbMetaDataJdbc30.java failed. This test has been already fixed by David. Please take a look at this patch. Thanks.
          Hide
          Daniel John Debrunner added a comment -

          The use of close() for close and reset is confusing (I know it's existing code).

          Your addition is now the only code in close() that actually generates new objects. This code
          will be called even when the statement is being closed in order that it no longer be used.
          This might have a performance impact, I don't know how often in a well behaved application
          this internal close method would be called. Once per statement execution or creation?

          I'm unclear on what this patch ('derby-210-patch3.diff') is addrressing. The comments for
          item 10) above don't actually help me much. Was the old code successfully re-using the old
          DRDAResultSet() but now for some reason we force it to use a new one every time?

          Show
          Daniel John Debrunner added a comment - The use of close() for close and reset is confusing (I know it's existing code). Your addition is now the only code in close() that actually generates new objects. This code will be called even when the statement is being closed in order that it no longer be used. This might have a performance impact, I don't know how often in a well behaved application this internal close method would be called. Once per statement execution or creation? I'm unclear on what this patch ('derby-210-patch3.diff') is addrressing. The comments for item 10) above don't actually help me much. Was the old code successfully re-using the old DRDAResultSet() but now for some reason we force it to use a new one every time?
          Hide
          Deepa Remesh added a comment -

          Attaching 'derby-210-patch4.diff' which changes the finalizer method in Statement and PreparedStatement classes in the client driver. Changes are:

          1. In PreparedStatement class, the finalizer was calling closeX method, which was doing:

          • Call super.closeX() ---> Statement.closeX()
          • Cleanup parameter objects - parameterMetaData_, sql_, parameters_ array
          • Remove the PreparedStatement from connection_.CommitAndRollbackListeners_ list

          Changes done by patch:

          • Add a new method markClosed() which will free client-side resources.
          • The new method is named markClosed() to keep it uniform with naming convention in superclass.
          • This method is called from close() and finalize() methods.
          • markClosed() method will call super.markClosed() to perform cleanup of parent class. It will cleanup the objects specific to PreparedStatement, which are ParameterMetaData and parameters. It also removes the PreparedStatement form the list in Connection object.

          2. In Statement class, the finalizer was calling closeX method, which was doing:

          • Close any open cursors for this statement on the server.
          • If result set is open on server, send CLSQRY to the server.
          • check if autocommit is required when closing result sets and flow a commit to server, if required
          • Call Statement.markClosed(), which does
          • Mark close the result sets on the client
          • If cursor name was set on the statement, remove it from Connection.clientCursorNameCache_
          • Call markClosed() on prepared statements for auto generated keys
          • Call markClosedOnServer(), which frees up the section. The freed section will be re-used by new statements.
          • Remove the Statement from Connection.openStatements_ list
          • Cleanup ResultSetMetaData

          Changes done by patch:

          • Move the cleanup of ResultSetMetaData and remove of Statement from Connection.openStatements_ list into markClosed() method. This will keep all client-side cleanup in markClosed().
          • Change the finalizer to just call markClosed(). This method frees up client-side resources and operates on synchronized collections. So I have removed the synchronize block from the finalizer.
          • The autocommit logic does not exist in the finalizer since only markClosed() is called from finalizer. This will avoid untimely commits which was causing the regression in the test lang/updatableResultSet.java

          With this patch, I ran derbynetclientmats with Sun JDK1.4.2 on Windows XP.

          Just before submitting this, I was re-reading the patch and noticed there is no need to override close and closeX method in PreparedStatement since these methods are doing exactly same things as the methods in Statement class. close() method in both classes just call closeX. And closeX does server-side cleanup and calls markClosed. And markClosed in PreparedStatement calls the super.markClosed. So there is no need for a close() and closeX() in PreparedStatement. This is possible because of the name change suggested by Bryan. Thanks again. I hope this change is okay. I will re-run derbynetclientmats and will submit a revised patch4.

          Bryan, if you get time, I would appreciate if you can look at this patch and let me know if it covers your comments.

          Show
          Deepa Remesh added a comment - Attaching 'derby-210-patch4.diff' which changes the finalizer method in Statement and PreparedStatement classes in the client driver. Changes are: 1. In PreparedStatement class, the finalizer was calling closeX method, which was doing: Call super.closeX() ---> Statement.closeX() Cleanup parameter objects - parameterMetaData_, sql_, parameters_ array Remove the PreparedStatement from connection_.CommitAndRollbackListeners_ list Changes done by patch: Add a new method markClosed() which will free client-side resources. The new method is named markClosed() to keep it uniform with naming convention in superclass. This method is called from close() and finalize() methods. markClosed() method will call super.markClosed() to perform cleanup of parent class. It will cleanup the objects specific to PreparedStatement, which are ParameterMetaData and parameters. It also removes the PreparedStatement form the list in Connection object. 2. In Statement class, the finalizer was calling closeX method, which was doing: Close any open cursors for this statement on the server. If result set is open on server, send CLSQRY to the server. check if autocommit is required when closing result sets and flow a commit to server, if required Call Statement.markClosed(), which does Mark close the result sets on the client If cursor name was set on the statement, remove it from Connection.clientCursorNameCache_ Call markClosed() on prepared statements for auto generated keys Call markClosedOnServer(), which frees up the section. The freed section will be re-used by new statements. Remove the Statement from Connection.openStatements_ list Cleanup ResultSetMetaData Changes done by patch: Move the cleanup of ResultSetMetaData and remove of Statement from Connection.openStatements_ list into markClosed() method. This will keep all client-side cleanup in markClosed(). Change the finalizer to just call markClosed(). This method frees up client-side resources and operates on synchronized collections. So I have removed the synchronize block from the finalizer. The autocommit logic does not exist in the finalizer since only markClosed() is called from finalizer. This will avoid untimely commits which was causing the regression in the test lang/updatableResultSet.java With this patch, I ran derbynetclientmats with Sun JDK1.4.2 on Windows XP. Just before submitting this, I was re-reading the patch and noticed there is no need to override close and closeX method in PreparedStatement since these methods are doing exactly same things as the methods in Statement class. close() method in both classes just call closeX. And closeX does server-side cleanup and calls markClosed. And markClosed in PreparedStatement calls the super.markClosed. So there is no need for a close() and closeX() in PreparedStatement. This is possible because of the name change suggested by Bryan. Thanks again. I hope this change is okay. I will re-run derbynetclientmats and will submit a revised patch4. Bryan, if you get time, I would appreciate if you can look at this patch and let me know if it covers your comments.
          Hide
          Deepa Remesh added a comment -

          Attaching 'derby-210-patch3.diff' which changes method DRDAStatement.close() in Network Server. The following line is added to the method:
          currentDrdaRs = new DRDAResultSet();

          This is required when statements get re-used without an explicit close done on the client side.

          With this patch, I ran derbynetmats and derbynetclientmats with Sun JDK1.4.2 on Windows XP. Please take a look at this patch. Thanks.

          Show
          Deepa Remesh added a comment - Attaching 'derby-210-patch3.diff' which changes method DRDAStatement.close() in Network Server. The following line is added to the method: currentDrdaRs = new DRDAResultSet(); This is required when statements get re-used without an explicit close done on the client side. With this patch, I ran derbynetmats and derbynetclientmats with Sun JDK1.4.2 on Windows XP. Please take a look at this patch. Thanks.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Deepa,

          I have attached a patch to DERBY-557 which adds a test case to your
          new test. In that patch, I enabled the test for the client driver, but
          inserted this into the test code to prevent your test case from being
          run:

          // Don't run under DerbyNetClient until DERBY-210 is fixed
          if (TestUtil.isDerbyNetClientFramework()) return;

          Hope I didn't mess things up for you.

          Show
          Knut Anders Hatlen added a comment - Hi Deepa, I have attached a patch to DERBY-557 which adds a test case to your new test. In that patch, I enabled the test for the client driver, but inserted this into the test code to prevent your test case from being run: // Don't run under DerbyNetClient until DERBY-210 is fixed if (TestUtil.isDerbyNetClientFramework()) return; Hope I didn't mess things up for you.
          Hide
          Kathey Marsden added a comment -

          I checked in 'derby-210-patch2.diff' to add a test in preparation for the fix for this issue.
          Date: Fri Feb 10 17:29:09 2006
          New Revision: 376904

          URL: http://svn.apache.org/viewcvs?rev=376904&view=rev

          Show
          Kathey Marsden added a comment - I checked in 'derby-210-patch2.diff' to add a test in preparation for the fix for this issue. Date: Fri Feb 10 17:29:09 2006 New Revision: 376904 URL: http://svn.apache.org/viewcvs?rev=376904&view=rev
          Hide
          Deepa Remesh added a comment -

          Attaching second patch 'derby-210-patch2.diff' which adds a test to jdbcapi suite for this issue. Patch does the following:

          • Adds a test derbyStress.java to jdbcapi suite. This test is based on the repro for this patch.
          • Excludes the new test from running with client driver, jcc and j9 because it gives out of memory error. Once derby-210 is resolved, this test should pass with client driver and the exclude must be removed from DerbyNetClient.exclude
          • Creates 'derbyStress_app.properties' with following property 'jvmflags=-Xmx64M' to guarantee the test fails on all machines with the current client driver.

          I ran jdbcapi suite with all frameworks using Sun JDK1.4.2 on Windows XP. Verified that the new test gets run with embedded and is skipped for client and jcc frameworks. Also ran the new test with j9 vms. Verified it passes with j9_foundation and is skipped for other j9s. Please take a look at this patch.

          Show
          Deepa Remesh added a comment - Attaching second patch 'derby-210-patch2.diff' which adds a test to jdbcapi suite for this issue. Patch does the following: Adds a test derbyStress.java to jdbcapi suite. This test is based on the repro for this patch. Excludes the new test from running with client driver, jcc and j9 because it gives out of memory error. Once derby-210 is resolved, this test should pass with client driver and the exclude must be removed from DerbyNetClient.exclude Creates 'derbyStress_app.properties' with following property 'jvmflags=-Xmx64M' to guarantee the test fails on all machines with the current client driver. I ran jdbcapi suite with all frameworks using Sun JDK1.4.2 on Windows XP. Verified that the new test gets run with embedded and is skipped for client and jcc frameworks. Also ran the new test with j9 vms. Verified it passes with j9_foundation and is skipped for other j9s. Please take a look at this patch.
          Hide
          Kathey Marsden added a comment -

          Submitted derby-210-patch1.diff

          Date: Fri Feb 10 14:00:11 2006
          New Revision: 376874

          URL: http://svn.apache.org/viewcvs?rev=376874&view=rev

          Show
          Kathey Marsden added a comment - Submitted derby-210-patch1.diff Date: Fri Feb 10 14:00:11 2006 New Revision: 376874 URL: http://svn.apache.org/viewcvs?rev=376874&view=rev
          Hide
          Deepa Remesh added a comment -

          Attaching a patch 'derby-210-patch1.diff' which removes the list 'RollbackOnlyListeners_' from org.apache.derby.client.am.Connection class since this is not being used. This is an incremental patch and does not solve the issue fully.

          While working on this issue, I found one of the causes of memory leaks is object references stored in the lists in the Connection class. On looking at the lists, I found that 'RollbackOnlyListeners' is not being used anywhere in the client code. Submitting this patch to remove this.

          Ran derbynetclientmats suite with Sun JDK1.4.2 on Windows XP. I see 1 failure in lang/wisconsin.java which is also seen in Ole's tests and reported in DERBY-937. I would appreciate if someone can look at this patch.

          Show
          Deepa Remesh added a comment - Attaching a patch 'derby-210-patch1.diff' which removes the list 'RollbackOnlyListeners_' from org.apache.derby.client.am.Connection class since this is not being used. This is an incremental patch and does not solve the issue fully. While working on this issue, I found one of the causes of memory leaks is object references stored in the lists in the Connection class. On looking at the lists, I found that 'RollbackOnlyListeners' is not being used anywhere in the client code. Submitting this patch to remove this. Ran derbynetclientmats suite with Sun JDK1.4.2 on Windows XP. I see 1 failure in lang/wisconsin.java which is also seen in Ole's tests and reported in DERBY-937 . I would appreciate if someone can look at this patch.
          Hide
          Daniel John Debrunner added a comment -

          My gut feeling is that this single patch is trying to do too much (10 described items). If any of these steps are independent I would strongly encourage you to submit them as separate patches. Looks like step 2) for example is independent and possibly a low risk change.
          Separate patches are good, they are easier to review, easier to explain, easier to manage during development as if you have several sets of changes in a client built on top of each other then as you code and re-code the most recent step you run the risk of corrupting the earlier changes. Getting the individual pieces out there earlier also gives them more exposure in the code line to additional testing and inspection. More people are likely to see the changes when they are in the code than when they are in a patch file.

          Show
          Daniel John Debrunner added a comment - My gut feeling is that this single patch is trying to do too much (10 described items). If any of these steps are independent I would strongly encourage you to submit them as separate patches. Looks like step 2) for example is independent and possibly a low risk change. Separate patches are good, they are easier to review, easier to explain, easier to manage during development as if you have several sets of changes in a client built on top of each other then as you code and re-code the most recent step you run the risk of corrupting the earlier changes. Getting the individual pieces out there earlier also gives them more exposure in the code line to additional testing and inspection. More people are likely to see the changes when they are in the code than when they are in a patch file.
          Hide
          Deepa Remesh added a comment -

          I am attaching a draft patch 'derby-210-v2-draft.diff' for review. It is not ready for commit. This patch contains changes in my first patch and some additional changes which I am describing below. I would appreciate if someone can read this and let me know if this is the right approach.

          The changes numbered 1-7 were done as part of first patch. I am copying it from my previous comment:

          1. Eliminates the below references to PreparedStatement objects by using WeakHashMap instead of LinkedList. When there are no other references to the keys in a WeakHashMap, they will get removed from the map and can thus get garbage-collected. They do not have to wait till the Connection object is collected.

          • 'openStatements_' in org.apache.derby.client.am.Connection
          • 'CommitAndRollbackListeners_' in org.apache.derby.client.am.Connection

          2. Removes the list 'RollbackOnlyListeners_' since this is not being used.

          3. Updates the following comment for openStatements_:
          // Since DERBY prepared statements must be re-prepared after a commit,
          // then we must traverse this list after a commit and notify statements
          // that they are now in an un-prepared state.
          final java.util.LinkedList openStatements_ = new java.util.LinkedList();

          In the code, I did not see this list being traversed after a commit to re-prepare statements. Also, I think this is not needed since Derby does not require re-prepare of statements after a commit. Currently, this list is used to close all open statements when the originating connection is closed.

          4. Removes all ResultSets from HashTable 'positionedUpdateCursorNameToResultSet_' in SectionManager. Only result sets of positioned update statements were being removed from this hashtable whereas all result sets were added. Because of this, client driver was holding on to result sets and statements even after rs.close() was called.

          5. Adds a test 'derbyStress.java' to jdbcapi suite. This test is based on the repro for this patch. Without this patch, it fails when run with client driver. Kathey had suggested in another mail that tests for client memory leak problems (DERBY-557, DERBY-210) can be added to same test. I did not see an existing test. So I created this new test. If DERBY-557 does not have a test, I think it can be added to this new test.

          6. Excludes the new test from running with jcc because jcc gives out of memory error.

          7. Creates 'derbyStress_app.properties' with following property 'jvmflags=-Xmx64M' to guarantee the test fails on all machines.

          With these changes, the memory leak mentioned in this issue was solved but it was causing an intermittent failure in lang/updatableResultSet.java as described in https://issues.apache.org/jira/browse/DERBY-210#action_12363439. To solve the regression caused by the first patch, I changed the finalizer in Statement and PreparedStatement classes to avoid network operations. Changes are:

          8. In PreparedStatement class, the finalizer was calling closeX method, which was doing:

          • Call super.closeX() ---> Statement.closeX()
          • Cleanup parameter objects - parameterMetaData_, sql_, parameters_ array
          • Remove the PreparedStatement from connection_.CommitAndRollbackListeners_ list

          Changes done by patch:

          • Move cleanup of objects to a new method cleanupParameters()
          • Call the new method cleanupParameters() from closeX() and finalize()
          • In finalize() method, call super.finalize() which is Statement.finalize() and it will do the remaining cleanup
          • Since WeakHashMap is used for Connection.CommitAndRollbackListeners_, the prepared statement object would already have been removed from this list before entering the finalizer. So there is no need to explicitly call a remove in the finalizer.

          9. In Statement class, the finalizer was calling closeX method, which was doing:

          • Close any open cursors for this statement on the server.
          • If result set is open on server, send CLSQRY to the server.
          • check if autocommit is required when closing result sets and flow a commit to server, if required
          • Call Statement.markClosed(), which does
          • Mark close the result sets on the client
          • If cursor name was set on the statement, remove it from Connection.clientCursorNameCache_
          • Call markClosed() on prepared statements for auto generated keys
          • Call markClosedOnServer(), which frees up the section. The freed section will be re-used by new statements.
          • Remove the Statement from Connection.openStatements_ list
          • Cleanup ResultSetMetaData

          Changes done by patch:

          • Move the cleanup of ResultSetMetaData into markClosed() method. This will keep all client-side cleanup in markClosed().
          • Change the finalizer to just call markClosed(). This method frees up client-side resources and operates on synchronized collections. So I have removed the synchronize block from the finalizer. markClosed() in turn calls markClosedOnServer(), which frees up a section. When the section gets re-used by new statement on client, server re-uses the same statement. newDrdaStatement in org.apache.derby.impl.drda.Database has this code:

          /**

          • Get a new DRDA statement and store it in the stmtTable if stortStmt is true
          • If possible recycle an existing statement
          • If we are asking for one with the same name it means it
          • was already closed.
          • @param pkgnamcsn Package name and section
          • @return DRDAStatement
            */
            protected DRDAStatement newDRDAStatement(Pkgnamcsn pkgnamcsn)
            throws SQLException
            Unknown macro: { DRDAStatement stmt = getDRDAStatement(pkgnamcsn); if (stmt != null) stmt.close(); else { stmt = new DRDAStatement(this); stmt.setPkgnamcsn(pkgnamcsn); storeStatement(stmt); } return stmt; }

          In the above method, close() is called before re-using the DRDAStatement. This will ensure the statement state is restored. DRDAStatement.close() also ensures the result set objects are closed and result set tables and all result sets are freed on server side. So there is no need to send explicit CLSQRY from client-side statement finalizer.

          • Since WeakHashMap is used for Connection.openStatements_, the statement object would already have been removed from this list before entering the finalizer. So there is no need to explicitly call a remove in the finalizer.
          • The autocommit logic does not exist in the finalizer since only markClosed() is called from finalizer. This will avoid untimely commits which was causing the regression in the test lang/updatableResultSet.java.

          With the above changes, I ran derbynetclientmats few times with jdk14 and jdk15. All tests passed except an intermittent failure in derbynet/prepStmt.java. This was happening in the test added for Jira125 where it calls rs.next(). I was getting the following exception:

          java.lang.StringIndexOutOfBoundsException: String index out of range: 25242
          at java.lang.String.checkBounds(String.java:372)
          at java.lang.String.<init>(String.java:404)
          at org.apache.derby.client.net.NetCursor.readFdocaString(NetCursor.java:753)
          at org.apache.derby.client.net.NetCursor.parseVCS(NetCursor.java:726)
          at org.apache.derby.client.net.NetCursor.parseSQLCAXGRP(NetCursor.java:693)
          at org.apache.derby.client.net.NetCursor.parseSQLCAGRP(NetCursor.java:622)
          at org.apache.derby.client.net.NetCursor.parseSQLCARD(NetCursor.java:595)
          at org.apache.derby.client.net.NetCursor.calculateColumnOffsetsForRow_(NetCursor.java:112)
          at org.apache.derby.client.am.Cursor.next(Cursor.java:165)
          at org.apache.derby.client.am.ResultSet.nextX(ResultSet.java:287)
          at org.apache.derby.client.am.ResultSet.next(ResultSet.java:259)
          at org.apache.derbyTesting.functionTests.tests.derbynet.prepStmt.jira125Test_a(prepStmt.java:904)
          at org.apache.derbyTesting.functionTests.tests.derbynet.prepStmt.jira125Test(prepStmt.java:820)
          at org.apache.derbyTesting.functionTests.tests.derbynet.prepStmt.main(prepStmt.java:313)

          It seemed to me that on the client side, the cleanup was happening correctly but on server side, the statement re-use was not happening correctly. DRDAStatement.close() was restoring all members except 'currentDrdaRs' and 'needsToSendParamData'.

          10. I added the following to DRDAStatement.close() method. This will ensure the previous result set is freed.
          currentDrdaRs = new DRDAResultSet();
          needsToSendParamData = false;

          With this change, I am running derbynetclientmats in a loop. I did not see any failures so far. I also ran the repro derbyStress.java with 50,000 prepared statements and did not get an out of memory error.

          I am attaching this interim draft patch to get feedback about the changes. Please let me know if this approach is okay. Thanks much for reading this.

          Show
          Deepa Remesh added a comment - I am attaching a draft patch 'derby-210-v2-draft.diff' for review. It is not ready for commit. This patch contains changes in my first patch and some additional changes which I am describing below. I would appreciate if someone can read this and let me know if this is the right approach. The changes numbered 1-7 were done as part of first patch. I am copying it from my previous comment: 1. Eliminates the below references to PreparedStatement objects by using WeakHashMap instead of LinkedList. When there are no other references to the keys in a WeakHashMap, they will get removed from the map and can thus get garbage-collected. They do not have to wait till the Connection object is collected. 'openStatements_' in org.apache.derby.client.am.Connection 'CommitAndRollbackListeners_' in org.apache.derby.client.am.Connection 2. Removes the list 'RollbackOnlyListeners_' since this is not being used. 3. Updates the following comment for openStatements_: // Since DERBY prepared statements must be re-prepared after a commit, // then we must traverse this list after a commit and notify statements // that they are now in an un-prepared state. final java.util.LinkedList openStatements_ = new java.util.LinkedList(); In the code, I did not see this list being traversed after a commit to re-prepare statements. Also, I think this is not needed since Derby does not require re-prepare of statements after a commit. Currently, this list is used to close all open statements when the originating connection is closed. 4. Removes all ResultSets from HashTable 'positionedUpdateCursorNameToResultSet_' in SectionManager. Only result sets of positioned update statements were being removed from this hashtable whereas all result sets were added. Because of this, client driver was holding on to result sets and statements even after rs.close() was called. 5. Adds a test 'derbyStress.java' to jdbcapi suite. This test is based on the repro for this patch. Without this patch, it fails when run with client driver. Kathey had suggested in another mail that tests for client memory leak problems ( DERBY-557 , DERBY-210 ) can be added to same test. I did not see an existing test. So I created this new test. If DERBY-557 does not have a test, I think it can be added to this new test. 6. Excludes the new test from running with jcc because jcc gives out of memory error. 7. Creates 'derbyStress_app.properties' with following property 'jvmflags=-Xmx64M' to guarantee the test fails on all machines. With these changes, the memory leak mentioned in this issue was solved but it was causing an intermittent failure in lang/updatableResultSet.java as described in https://issues.apache.org/jira/browse/DERBY-210#action_12363439 . To solve the regression caused by the first patch, I changed the finalizer in Statement and PreparedStatement classes to avoid network operations. Changes are: 8. In PreparedStatement class, the finalizer was calling closeX method, which was doing: Call super.closeX() ---> Statement.closeX() Cleanup parameter objects - parameterMetaData_, sql_, parameters_ array Remove the PreparedStatement from connection_.CommitAndRollbackListeners_ list Changes done by patch: Move cleanup of objects to a new method cleanupParameters() Call the new method cleanupParameters() from closeX() and finalize() In finalize() method, call super.finalize() which is Statement.finalize() and it will do the remaining cleanup Since WeakHashMap is used for Connection.CommitAndRollbackListeners_, the prepared statement object would already have been removed from this list before entering the finalizer. So there is no need to explicitly call a remove in the finalizer. 9. In Statement class, the finalizer was calling closeX method, which was doing: Close any open cursors for this statement on the server. If result set is open on server, send CLSQRY to the server. check if autocommit is required when closing result sets and flow a commit to server, if required Call Statement.markClosed(), which does Mark close the result sets on the client If cursor name was set on the statement, remove it from Connection.clientCursorNameCache_ Call markClosed() on prepared statements for auto generated keys Call markClosedOnServer(), which frees up the section. The freed section will be re-used by new statements. Remove the Statement from Connection.openStatements_ list Cleanup ResultSetMetaData Changes done by patch: Move the cleanup of ResultSetMetaData into markClosed() method. This will keep all client-side cleanup in markClosed(). Change the finalizer to just call markClosed(). This method frees up client-side resources and operates on synchronized collections. So I have removed the synchronize block from the finalizer. markClosed() in turn calls markClosedOnServer(), which frees up a section. When the section gets re-used by new statement on client, server re-uses the same statement. newDrdaStatement in org.apache.derby.impl.drda.Database has this code: /** Get a new DRDA statement and store it in the stmtTable if stortStmt is true If possible recycle an existing statement If we are asking for one with the same name it means it was already closed. @param pkgnamcsn Package name and section @return DRDAStatement */ protected DRDAStatement newDRDAStatement(Pkgnamcsn pkgnamcsn) throws SQLException Unknown macro: { DRDAStatement stmt = getDRDAStatement(pkgnamcsn); if (stmt != null) stmt.close(); else { stmt = new DRDAStatement(this); stmt.setPkgnamcsn(pkgnamcsn); storeStatement(stmt); } return stmt; } In the above method, close() is called before re-using the DRDAStatement. This will ensure the statement state is restored. DRDAStatement.close() also ensures the result set objects are closed and result set tables and all result sets are freed on server side. So there is no need to send explicit CLSQRY from client-side statement finalizer. Since WeakHashMap is used for Connection.openStatements_, the statement object would already have been removed from this list before entering the finalizer. So there is no need to explicitly call a remove in the finalizer. The autocommit logic does not exist in the finalizer since only markClosed() is called from finalizer. This will avoid untimely commits which was causing the regression in the test lang/updatableResultSet.java. With the above changes, I ran derbynetclientmats few times with jdk14 and jdk15. All tests passed except an intermittent failure in derbynet/prepStmt.java. This was happening in the test added for Jira125 where it calls rs.next(). I was getting the following exception: java.lang.StringIndexOutOfBoundsException: String index out of range: 25242 at java.lang.String.checkBounds(String.java:372) at java.lang.String.<init>(String.java:404) at org.apache.derby.client.net.NetCursor.readFdocaString(NetCursor.java:753) at org.apache.derby.client.net.NetCursor.parseVCS(NetCursor.java:726) at org.apache.derby.client.net.NetCursor.parseSQLCAXGRP(NetCursor.java:693) at org.apache.derby.client.net.NetCursor.parseSQLCAGRP(NetCursor.java:622) at org.apache.derby.client.net.NetCursor.parseSQLCARD(NetCursor.java:595) at org.apache.derby.client.net.NetCursor.calculateColumnOffsetsForRow_(NetCursor.java:112) at org.apache.derby.client.am.Cursor.next(Cursor.java:165) at org.apache.derby.client.am.ResultSet.nextX(ResultSet.java:287) at org.apache.derby.client.am.ResultSet.next(ResultSet.java:259) at org.apache.derbyTesting.functionTests.tests.derbynet.prepStmt.jira125Test_a(prepStmt.java:904) at org.apache.derbyTesting.functionTests.tests.derbynet.prepStmt.jira125Test(prepStmt.java:820) at org.apache.derbyTesting.functionTests.tests.derbynet.prepStmt.main(prepStmt.java:313) It seemed to me that on the client side, the cleanup was happening correctly but on server side, the statement re-use was not happening correctly. DRDAStatement.close() was restoring all members except 'currentDrdaRs' and 'needsToSendParamData'. 10. I added the following to DRDAStatement.close() method. This will ensure the previous result set is freed. currentDrdaRs = new DRDAResultSet(); needsToSendParamData = false; With this change, I am running derbynetclientmats in a loop. I did not see any failures so far. I also ran the repro derbyStress.java with 50,000 prepared statements and did not get an out of memory error. I am attaching this interim draft patch to get feedback about the changes. Please let me know if this approach is okay. Thanks much for reading this.
          Hide
          John H. Embretsen added a comment -

          Uploaded graphs showing the Sun 1.5 JVM's utilization of Permanent Generation space in the Java heap (i.e., the JVM running the Derby Network Server) when running the DOTS test case ATCJ2 using Derby @ SVN 373478 (Jan 30 2006), with and without the derby-210.diff patch uploaded by Deepa. Statistics were obtained by using the "jstat" monitoring tool.

          The heap size was fixed at 128 MB, and the Permanent Generation Space was fixed at 64 MB. The attached graphs show the PermSpace because that was the part of the heap that was having the most trouble during the DOTS test run.

          • DOTS_ATCJ2_Derby-noPatch.png

          shows PermSpace utilization on the server side without the patch, the first 20 hours (72k seconds) of the test run. The Server JVM threw an "OutOfMemoryError: PermGen space" after approximately 3 hours.

          • DOTS_ATCJ2_Derby-withPatch.png

          shows PermSpace utilization on the server side with the patch, the first 20 hours of the test run. No OutOfMemoryErrors were thrown within this time period.

          In other words, the patch seems to provide significant improvement to Derby robustness with regards to cases where statements are not always explicitly closed by the application using the Derby client. The garbage collector is able to collect much more garbage with the patch than without.

          For details on the DOTS test case and results obtained by running it, please refer to the following thread on the derby-user mailing list:
          http://www.nabble.com/OutOfMemoryErrors-when-testing-Derby-with-DOTS-t1010027.html

          When it comes to GC performance, it seems that with the patch, the garbage collector spends less time doing (fewer) major (full) GCs, but more time doing (more) minor GCs. Minor GC is a lot cheaper than major GC, since only the "young" generations are garbage collected, not the entire heap (young + tenured + permanent space).

          I hope this patch gets (re-)committed once the current issues are resolved. Thanks, Deepa!

          Show
          John H. Embretsen added a comment - Uploaded graphs showing the Sun 1.5 JVM's utilization of Permanent Generation space in the Java heap (i.e., the JVM running the Derby Network Server) when running the DOTS test case ATCJ2 using Derby @ SVN 373478 (Jan 30 2006), with and without the derby-210.diff patch uploaded by Deepa. Statistics were obtained by using the "jstat" monitoring tool. The heap size was fixed at 128 MB, and the Permanent Generation Space was fixed at 64 MB. The attached graphs show the PermSpace because that was the part of the heap that was having the most trouble during the DOTS test run. DOTS_ATCJ2_Derby-noPatch.png shows PermSpace utilization on the server side without the patch, the first 20 hours (72k seconds) of the test run. The Server JVM threw an "OutOfMemoryError: PermGen space" after approximately 3 hours. DOTS_ATCJ2_Derby-withPatch.png shows PermSpace utilization on the server side with the patch, the first 20 hours of the test run. No OutOfMemoryErrors were thrown within this time period. In other words, the patch seems to provide significant improvement to Derby robustness with regards to cases where statements are not always explicitly closed by the application using the Derby client. The garbage collector is able to collect much more garbage with the patch than without. For details on the DOTS test case and results obtained by running it, please refer to the following thread on the derby-user mailing list: http://www.nabble.com/OutOfMemoryErrors-when-testing-Derby-with-DOTS-t1010027.html When it comes to GC performance, it seems that with the patch, the garbage collector spends less time doing (fewer) major (full) GCs, but more time doing (more) minor GCs. Minor GC is a lot cheaper than major GC, since only the "young" generations are garbage collected, not the entire heap (young + tenured + permanent space). I hope this patch gets (re-)committed once the current issues are resolved. Thanks, Deepa!
          Hide
          Kathey Marsden added a comment -

          I agree that garbage collection should not drive a commit, for the reasons stated by you and Dan and the fact that it is just to scary. It would mean non-holdable cursors will close, some transaction in progress can just commit out of the blue, etc, so it sounds like not a good thing to me.

          In terms of a repro, I would think you could repro the gc commit with your fix by
          1) make sure autocommit is on
          2) Execute a select with a holdable cursor and do not select the last row.
          3) set the Statement reference to null.
          4) Execute a select with a non-holdable cursor. Do not fetch the last row.
          5) force gc.
          6) Try to select the next row for the non-holdable cursor. It should say the cursor is closed because the gc would

          On whether a Statement.close() should drive a commit ..
          From what was described earlier, the commit comes from the ResultSet associated with the statement getting closed, not the statement itself.
          I am loathe to enter this conversation again, as the problem seems intractable so will offer only quotes for evaluation by the reader.

          From section 10.1

          For Select statements, the statement is complete when the associated result set is closed. The result set is closed as soon as one of the following occurs:

          • all of the rows have been retrieved
          • the associated Statement object is re-executed
          • another Statement object is executed on the same connection

          The javadoc for Statement.close()

          From Statement javadoc
          When a Statement object is closed, its current ResultSet object, if one exists, is also closed.

          The javadoc for ResultSet.close()
          A ResultSet object is automatically closed by the Statement object that generated it when that Statement object is closed, re-executed, or is used to retrieve the next result from a sequence of multiple results. A ResultSet object is also automatically closed when it is garbage collected.

          Show
          Kathey Marsden added a comment - I agree that garbage collection should not drive a commit, for the reasons stated by you and Dan and the fact that it is just to scary. It would mean non-holdable cursors will close, some transaction in progress can just commit out of the blue, etc, so it sounds like not a good thing to me. In terms of a repro, I would think you could repro the gc commit with your fix by 1) make sure autocommit is on 2) Execute a select with a holdable cursor and do not select the last row. 3) set the Statement reference to null. 4) Execute a select with a non-holdable cursor. Do not fetch the last row. 5) force gc. 6) Try to select the next row for the non-holdable cursor. It should say the cursor is closed because the gc would On whether a Statement.close() should drive a commit .. From what was described earlier, the commit comes from the ResultSet associated with the statement getting closed, not the statement itself. I am loathe to enter this conversation again, as the problem seems intractable so will offer only quotes for evaluation by the reader. From section 10.1 For Select statements, the statement is complete when the associated result set is closed. The result set is closed as soon as one of the following occurs: all of the rows have been retrieved the associated Statement object is re-executed another Statement object is executed on the same connection The javadoc for Statement.close() From Statement javadoc When a Statement object is closed, its current ResultSet object, if one exists, is also closed. The javadoc for ResultSet.close() A ResultSet object is automatically closed by the Statement object that generated it when that Statement object is closed, re-executed, or is used to retrieve the next result from a sequence of multiple results. A ResultSet object is also automatically closed when it is garbage collected.
          Hide
          Deepa Remesh added a comment -

          Thanks Kathey, Bryan and Dan for your comments.

          As Bryan pointed out, the javadoc for Statement.close mentions "A Statement object is automatically closed when it is garbage collected". I did not see this mentioned in the JDBC spec though. However, I still think close() and finalize() must be different. close() methods can throw SQLExceptions. javadoc for finalize() says this "Any exception thrown by the finalize method causes the finalization of this object to be halted, but is otherwise ignored." So I think we should just ensure that we can clean up all resources when finalize() gets run. Another reason which Dan pointed out is that the order of finalizer execution is not guaranteed.

          Dan, after reading through Section 10.1, I think we do not need to auto-commit when we close a statement.

          Kathey, I tried to force gc to repro this problem on other VMs. But I was not successful in creating another repro. The thing is that I am not able to repro this at all anymore, not even on jdk1.5. After I updated my local svn workspace and applied my original patch, I do not get this problem anymore. However, it consistently repoduces with jdk1.5 when running with the revision where the patch was committed (svn 369612). So I am doing changes on svn revision 369612 and running tests to verify this problem does not occur. Then I'll update to latest svn revision, do the same changes and run tests. I cannot think of any other way to verify this.

          Show
          Deepa Remesh added a comment - Thanks Kathey, Bryan and Dan for your comments. As Bryan pointed out, the javadoc for Statement.close mentions "A Statement object is automatically closed when it is garbage collected". I did not see this mentioned in the JDBC spec though. However, I still think close() and finalize() must be different. close() methods can throw SQLExceptions. javadoc for finalize() says this "Any exception thrown by the finalize method causes the finalization of this object to be halted, but is otherwise ignored." So I think we should just ensure that we can clean up all resources when finalize() gets run. Another reason which Dan pointed out is that the order of finalizer execution is not guaranteed. Dan, after reading through Section 10.1, I think we do not need to auto-commit when we close a statement. Kathey, I tried to force gc to repro this problem on other VMs. But I was not successful in creating another repro. The thing is that I am not able to repro this at all anymore, not even on jdk1.5. After I updated my local svn workspace and applied my original patch, I do not get this problem anymore. However, it consistently repoduces with jdk1.5 when running with the revision where the patch was committed (svn 369612). So I am doing changes on svn revision 369612 and running tests to verify this problem does not occur. Then I'll update to latest svn revision, do the same changes and run tests. I cannot think of any other way to verify this.
          Hide
          Daniel John Debrunner added a comment -

          Couple of issues here:

          1) Doing anything complex in a finalizer can be trouble, as the order of finalizer execution is not guaranteed and so any synchronization can lead to deadlocks. The embedded engine works around this by having the finalizer just mark some object as not being used any more and some later action on the connection will also cleanup the finailzed object.

          2) I think the actions of Statement.close() need to be investigated, I didn't think that Statement.close() had anyrelationship to automcommit and issuing a COMMIT. Section 10.1 of JDBC 3.0 has no mention of Statement.close().

          Show
          Daniel John Debrunner added a comment - Couple of issues here: 1) Doing anything complex in a finalizer can be trouble, as the order of finalizer execution is not guaranteed and so any synchronization can lead to deadlocks. The embedded engine works around this by having the finalizer just mark some object as not being used any more and some later action on the connection will also cleanup the finailzed object. 2) I think the actions of Statement.close() need to be investigated, I didn't think that Statement.close() had anyrelationship to automcommit and issuing a COMMIT. Section 10.1 of JDBC 3.0 has no mention of Statement.close().
          Hide
          Bryan Pendleton added a comment -

          Hi Deepa, thanks very much for posting your research, it was extremely interesting.

          I'm not sure I agree with Kathey, though. My expectation was that garbage collecting a Statement is the same as close, and so I think your implementation is correct and the test is not written precisely enough.

          Here's an interesting post from a Sun web page:
          http://forum.java.sun.com/thread.jspa?threadID=689539&messageID=4018996

          It seems to me like the next step is to try to study the specs and clarify the requirements: what exactly is it supposed to mean to garbage collect a Statement object?

          Show
          Bryan Pendleton added a comment - Hi Deepa, thanks very much for posting your research, it was extremely interesting. I'm not sure I agree with Kathey, though. My expectation was that garbage collecting a Statement is the same as close, and so I think your implementation is correct and the test is not written precisely enough. Here's an interesting post from a Sun web page: http://forum.java.sun.com/thread.jspa?threadID=689539&messageID=4018996 It seems to me like the next step is to try to study the specs and clarify the requirements: what exactly is it supposed to mean to garbage collect a Statement object?
          Hide
          Kathey Marsden added a comment -

          It sounds to me like you are on the right track. I agree, the finalize method is not the same as a close and should not send a commit or otherwise affect data. It should only cleanup resources. This is an interesting test, to rollback in autocommit mode. You could probably force a gc() right befor e the rollback to make the issue happen with other jvms.

          Show
          Kathey Marsden added a comment - It sounds to me like you are on the right track. I agree, the finalize method is not the same as a close and should not send a commit or otherwise affect data. It should only cleanup resources. This is an interesting test, to rollback in autocommit mode. You could probably force a gc() right befor e the rollback to make the issue happen with other jvms.
          Hide
          Deepa Remesh added a comment -

          I looked into the failure in lang/updatableResultSet.java with derbynetclient on JDK 1.5 and this is what I found:

          This test was failing after my patch for derby-210 was committed. The failure occurs in the following part of the test. I have included some of my thoughts as comments starting with "// *****" in the below code:
          --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
          System.out.println("Positive Test34 - in autocommit mode, check that updateRow and deleteRow does not commit");
          conn.setAutoCommit(true);

          // First try deleteRow and updateRow on first row of result set
          reloadData();
          System.out.println(" Contents before changes to first row in RS:");
          dumpRS(stmt.executeQuery("select * from t1"));
          stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE);
          rs = stmt.executeQuery("SELECT * FROM t1 FOR UPDATE");
          rs.next();
          rs.deleteRow();
          conn.rollback();
          rs.close();
          System.out.println(" Make sure the contents of table are unchanged:");
          dumpRS(stmt.executeQuery("select * from t1"));
          stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE);
          rs = stmt.executeQuery("SELECT * FROM t1 FOR UPDATE");
          rs.next();
          rs.updateInt(1,-rs.getInt(1));
          rs.updateRow();

          // ****************************************************************************************************************
          // ************************************* A commit was occurring here ******************************************
          // ****************************************************************************************************************

          conn.rollback();
          rs.close();
          System.out.println(" Make sure the contents of table are unchanged:");
          dumpRS(stmt.executeQuery("select * from t1"));

          // ***************************************************************************************************************
          // ******************************* Test was failing since the row gets changed due to the commit ***********
          // ***************************************************************************************************************

          // Now try the same on the last row in the result set
          reloadData();
          stmt = conn.createStatement();
          rs = stmt.executeQuery("SELECT COUNT FROM t1");
          rs.next();
          int count = rs.getInt(1);
          rs.close();

          System.out.println(" Contents before changes to last row in RS:");
          dumpRS(stmt.executeQuery("select * from t1"));
          stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE);
          rs = stmt.executeQuery("SELECT * FROM t1 FOR UPDATE");
          for (int j = 0; j < count; j++)

          { rs.next(); }
          rs.deleteRow();
          conn.rollback();
          rs.close();
          System.out.println(" Make sure the contents of table are unchanged:");
          dumpRS(stmt.executeQuery("select * from t1"));

          stmt = conn.createStatement();
          rs = stmt.executeQuery("SELECT COUNT FROM t1");
          rs.next();
          count = rs.getInt(1);
          rs.close();

          stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE);
          rs = stmt.executeQuery("SELECT * FROM t1 FOR UPDATE");
          for (int j = 0; j < count; j++) { rs.next(); }

          rs.updateInt(1,-rs.getInt(1));
          rs.updateRow();

          // *******************************************************************************************************************************
          // ************* No commit was occurring here. This made me think the commit is being sent from some other place
          // *******************************************************************************************************************************

          conn.rollback();
          rs.close();
          System.out.println(" Make sure the contents of table are unchanged:");
          dumpRS(stmt.executeQuery("select * from t1"));

          // ****************************************************************************************************************
          // ******************************* Test passes here, which means the failure is sporadic *******************
          // ****************************************************************************************************************

          --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

          I could see in Network Server trace that it was getting a commit command (RDBCMM) in between the rs.updateRow and conn.rollback() only in one particular case. I tried to trace the client to see from where this commit was coming from. But turning on client trace made this problem go away. I tried to create a smaller repro for the problem but was not able to simulate this failure. Also, the test was failing only on "Sun JDK 1.5.0_02". If I even slightly change the order in the test, this problem goes away. So the failure was sporadic but happens consistently with the updatableResultSet test on Sun JDK 1.5.0_02. I am glad we caught this problem on this jvm. Because after debugging, I realize this can occur on any jvm/platform. It just depends on when the garbage collector (GC) runs.

          Since my patch changed when statements get garbage collected, I walked through that path and found that it was the GC that was sending this commit to network server. When a statement object gets ready for garbage collection, GC calls it's finalize() method. The finalize() method in Statement class actually calls closeX() which does the same things as Statement.close() method. When autocommit is on, in some cases, this will call connection_.writeAutoCommit() as shown by this code snippet from Statement.writeCloseResultSets. This causes a commit to be sent to network server and if this happens at wrong time, the output becomes erroneous.
          --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

          if (connection_.autoCommit_ && requiresAutocommit && isAutoCommittableStatement_) {
          connection_.writeAutoCommit();
          --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

          This problem will not occur without my patch because of this:

          --------------------------------
          Without my patch:
          --------------------------------
          Statements get free for garbage collection:

          • ONLY after they are explicitly closed (Statement.close() has been called) or the connection itself is closed.
            AND
          • when the user application has no more references to the Statement

          In this case, when GC claims a statement object, it would have been closed already. Because of this, when finalize() --> closeX() is called, openOnClient_ is false and hence finalize() method returns without calling closeX().

          --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
          protected void finalize() throws java.lang.Throwable {
          if (agent_.loggingEnabled())

          { agent_.logWriter_.traceEntry(this, "finalize"); }

          if (openOnClient_) {
          synchronized (connection_)

          { closeX(); }

          }
          super.finalize();
          }
          --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

          --------------------------------
          With my patch:
          --------------------------------
          Statements get free for garbage collection:

          • when the user application has no more references to the Statement
            which I think is the correct behavior.

          In this case, when GC claims a statement object, openOnClient_ will be true if no Statement.close() has been called previously. Because of this, finalize() --> closeX() is called. And it goes through the entire chain of actions which happen in closeX(). One of this is a call to connection_.writeAutoCommit() which sends a RDBCMM to the server and server does a commit. By chance if the commit message is recieved after the execution of a statement which is not supposed to be autocommitted (like the updateRow statement), this causes erroneous behavior.

          I verified using printlns that this commit is getting sent from Statement.writeCloseResultSets. Also ran the test after removing call to closeX() from Statement.finalize(), in which case this problem goes away. This call to closeX() seems to be the cause of the problem. I am yet to figure out what subset of actions need to be done in finalize().

          --------------------------------
          Summary:
          --------------------------------
          I think my patch for derby-210 is doing the right thing by preventing statement leaks. However, with my patch, some paths of code are getting visited for first time from the GC thread. This is causing sporadic failures because the finalize method in statement class (I think) does a lot more things than required, one of them being sending commit message to the server. I think the statement finalizer should not be sending anything to the network server, at least not at connection level. I have to look some more into the finalizers in client classes and what happens at server side before I can re-submit a patch for this. I thought I'll post this to see what others think.

          I would appreciate any feedback on this.

          Show
          Deepa Remesh added a comment - I looked into the failure in lang/updatableResultSet.java with derbynetclient on JDK 1.5 and this is what I found: This test was failing after my patch for derby-210 was committed. The failure occurs in the following part of the test. I have included some of my thoughts as comments starting with "// *****" in the below code: -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- System.out.println("Positive Test34 - in autocommit mode, check that updateRow and deleteRow does not commit"); conn.setAutoCommit(true); // First try deleteRow and updateRow on first row of result set reloadData(); System.out.println(" Contents before changes to first row in RS:"); dumpRS(stmt.executeQuery("select * from t1")); stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE); rs = stmt.executeQuery("SELECT * FROM t1 FOR UPDATE"); rs.next(); rs.deleteRow(); conn.rollback(); rs.close(); System.out.println(" Make sure the contents of table are unchanged:"); dumpRS(stmt.executeQuery("select * from t1")); stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE); rs = stmt.executeQuery("SELECT * FROM t1 FOR UPDATE"); rs.next(); rs.updateInt(1,-rs.getInt(1)); rs.updateRow(); // **************************************************************************************************************** // ************************************* A commit was occurring here ****************************************** // **************************************************************************************************************** conn.rollback(); rs.close(); System.out.println(" Make sure the contents of table are unchanged:"); dumpRS(stmt.executeQuery("select * from t1")); // *************************************************************************************************************** // ******************************* Test was failing since the row gets changed due to the commit *********** // *************************************************************************************************************** // Now try the same on the last row in the result set reloadData(); stmt = conn.createStatement(); rs = stmt.executeQuery("SELECT COUNT FROM t1"); rs.next(); int count = rs.getInt(1); rs.close(); System.out.println(" Contents before changes to last row in RS:"); dumpRS(stmt.executeQuery("select * from t1")); stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE); rs = stmt.executeQuery("SELECT * FROM t1 FOR UPDATE"); for (int j = 0; j < count; j++) { rs.next(); } rs.deleteRow(); conn.rollback(); rs.close(); System.out.println(" Make sure the contents of table are unchanged:"); dumpRS(stmt.executeQuery("select * from t1")); stmt = conn.createStatement(); rs = stmt.executeQuery("SELECT COUNT FROM t1"); rs.next(); count = rs.getInt(1); rs.close(); stmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE); rs = stmt.executeQuery("SELECT * FROM t1 FOR UPDATE"); for (int j = 0; j < count; j++) { rs.next(); } rs.updateInt(1,-rs.getInt(1)); rs.updateRow(); // ******************************************************************************************************************************* // ************* No commit was occurring here. This made me think the commit is being sent from some other place // ******************************************************************************************************************************* conn.rollback(); rs.close(); System.out.println(" Make sure the contents of table are unchanged:"); dumpRS(stmt.executeQuery("select * from t1")); // **************************************************************************************************************** // ******************************* Test passes here, which means the failure is sporadic ******************* // **************************************************************************************************************** -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- I could see in Network Server trace that it was getting a commit command (RDBCMM) in between the rs.updateRow and conn.rollback() only in one particular case. I tried to trace the client to see from where this commit was coming from. But turning on client trace made this problem go away. I tried to create a smaller repro for the problem but was not able to simulate this failure. Also, the test was failing only on "Sun JDK 1.5.0_02". If I even slightly change the order in the test, this problem goes away. So the failure was sporadic but happens consistently with the updatableResultSet test on Sun JDK 1.5.0_02. I am glad we caught this problem on this jvm. Because after debugging, I realize this can occur on any jvm/platform. It just depends on when the garbage collector (GC) runs. Since my patch changed when statements get garbage collected, I walked through that path and found that it was the GC that was sending this commit to network server. When a statement object gets ready for garbage collection, GC calls it's finalize() method. The finalize() method in Statement class actually calls closeX() which does the same things as Statement.close() method. When autocommit is on, in some cases, this will call connection_.writeAutoCommit() as shown by this code snippet from Statement.writeCloseResultSets. This causes a commit to be sent to network server and if this happens at wrong time, the output becomes erroneous. -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- if (connection_.autoCommit_ && requiresAutocommit && isAutoCommittableStatement_) { connection_.writeAutoCommit(); -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- This problem will not occur without my patch because of this: -------------------------------- Without my patch: -------------------------------- Statements get free for garbage collection: ONLY after they are explicitly closed (Statement.close() has been called) or the connection itself is closed. AND when the user application has no more references to the Statement In this case, when GC claims a statement object, it would have been closed already. Because of this, when finalize() --> closeX() is called, openOnClient_ is false and hence finalize() method returns without calling closeX(). -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- protected void finalize() throws java.lang.Throwable { if (agent_.loggingEnabled()) { agent_.logWriter_.traceEntry(this, "finalize"); } if (openOnClient_) { synchronized (connection_) { closeX(); } } super.finalize(); } -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- -------------------------------- With my patch: -------------------------------- Statements get free for garbage collection: when the user application has no more references to the Statement which I think is the correct behavior. In this case, when GC claims a statement object, openOnClient_ will be true if no Statement.close() has been called previously. Because of this, finalize() --> closeX() is called. And it goes through the entire chain of actions which happen in closeX(). One of this is a call to connection_.writeAutoCommit() which sends a RDBCMM to the server and server does a commit. By chance if the commit message is recieved after the execution of a statement which is not supposed to be autocommitted (like the updateRow statement), this causes erroneous behavior. I verified using printlns that this commit is getting sent from Statement.writeCloseResultSets. Also ran the test after removing call to closeX() from Statement.finalize(), in which case this problem goes away. This call to closeX() seems to be the cause of the problem. I am yet to figure out what subset of actions need to be done in finalize(). -------------------------------- Summary: -------------------------------- I think my patch for derby-210 is doing the right thing by preventing statement leaks. However, with my patch, some paths of code are getting visited for first time from the GC thread. This is causing sporadic failures because the finalize method in statement class (I think) does a lot more things than required, one of them being sending commit message to the server. I think the statement finalizer should not be sending anything to the network server, at least not at connection level. I have to look some more into the finalizers in client classes and what happens at server side before I can re-submit a patch for this. I thought I'll post this to see what others think. I would appreciate any feedback on this.
          Hide
          Kathey Marsden added a comment -

          I reverted this patch and ran derbynetclientmats
          I saw two failures also in nightlies:

          jdbcapi/metadata.java
          lang/syscat.sql

          Date: Wed Jan 18 13:55:36 2006
          New Revision: 370275

          URL: http://svn.apache.org/viewcvs?rev=370275&view=rev

          Show
          Kathey Marsden added a comment - I reverted this patch and ran derbynetclientmats I saw two failures also in nightlies: jdbcapi/metadata.java lang/syscat.sql Date: Wed Jan 18 13:55:36 2006 New Revision: 370275 URL: http://svn.apache.org/viewcvs?rev=370275&view=rev
          Hide
          Deepa Remesh added a comment -

          This patch is causing a regression in the nightlies with Sun JDK1.5. The test lang/updatableResultset.java is failing in derbynetclientmats on this jvm.

          Kathey, can you please revert the patch from trunk? I will try to see what is causing this failure and upload a new patch.

          Show
          Deepa Remesh added a comment - This patch is causing a regression in the nightlies with Sun JDK1.5. The test lang/updatableResultset.java is failing in derbynetclientmats on this jvm. Kathey, can you please revert the patch from trunk? I will try to see what is causing this failure and upload a new patch.
          Hide
          Deepa Remesh added a comment -

          Attaching a patch 'derby-210_10.1.diff' for 10.1 branch. I could not merge the changes because of conflict in 'DerbyNet.exclude' file. Also I had to add the method 'cleanUpTest' from TestUtil.java to 10.1 as my test uses this new method.

          Ran derbyall in 10.1 with Sun JDK 1.4.2 on Windows XP. Only 1 intermittent failure in lang/ConcurrentImplicitCreateSchema.java.

          Show
          Deepa Remesh added a comment - Attaching a patch 'derby-210_10.1.diff' for 10.1 branch. I could not merge the changes because of conflict in 'DerbyNet.exclude' file. Also I had to add the method 'cleanUpTest' from TestUtil.java to 10.1 as my test uses this new method. Ran derbyall in 10.1 with Sun JDK 1.4.2 on Windows XP. Only 1 intermittent failure in lang/ConcurrentImplicitCreateSchema.java.
          Hide
          Kathey Marsden added a comment -

          Committed this patch.

          Date: Mon Jan 16 16:10:58 2006
          New Revision: 369612

          URL: http://svn.apache.org/viewcvs?rev=369612&view=rev

          Thank you Deepa for resolving this important client issue. I think that this might be a good candidate to port to 10.1. Do you see any issues/risks associated with porting this fix to 10.1?

          Also I will add some additional cleanup comments to DERBY-817.

          Show
          Kathey Marsden added a comment - Committed this patch. Date: Mon Jan 16 16:10:58 2006 New Revision: 369612 URL: http://svn.apache.org/viewcvs?rev=369612&view=rev Thank you Deepa for resolving this important client issue. I think that this might be a good candidate to port to 10.1. Do you see any issues/risks associated with porting this fix to 10.1? Also I will add some additional cleanup comments to DERBY-817 .
          Hide
          Deepa Remesh added a comment -

          I am uploading a combined patch 'derby-210.diff' which solves the memory leak. As Bryan suggested, I am uploading this patch and will open jira issues for other optimizations. Patch does the following:

          • Eliminates the below references to PreparedStatement objects by using WeakHashMap instead of LinkedList. When there are no other references to the keys in a WeakHashMap, they will get removed from the map and can thus get garbage-collected. They do not have to wait till the Connection object is collected.
          • 'openStatements_' in org.apache.derby.client.am.Connection
          • 'CommitAndRollbackListeners_' in org.apache.derby.client.am.Connection
          • Removes the list 'RollbackOnlyListeners_' since this is not being used.
          • Updates the following comment for openStatements_:
            // Since DERBY prepared statements must be re-prepared after a commit,
            // then we must traverse this list after a commit and notify statements
            // that they are now in an un-prepared state.
            final java.util.LinkedList openStatements_ = new java.util.LinkedList();

          In the code, I did not see this list being traversed after a commit to re-prepare statements. Also, I think this is not needed since Derby does not require re-prepare of statements after a commit. Currently, this list is used to close all open statements when the originating connection is closed.

          • Removes all ResultSets from HashTable 'positionedUpdateCursorNameToResultSet_' in SectionManager. Only result sets of positioned update statements were being removed from this hashtable whereas all result sets were added. Because of this, client driver was holding on to result sets and statements even after rs.close() was called.
          • Adds a test 'derbyStress.java' to jdbcapi suite. This test is based on the repro for this patch. Without this patch, it fails when run with client driver. Kathey had suggested in another mail that tests for client memory leak problems (DERBY-557, DERBY-210) can be added to same test. I did not see an existing test. So I created this new test. If DERBY-557 does not have a test, I think it can be added to this new test.
          • Excludes the new test from running with jcc because jcc gives out of memory error.
          • Creates 'derbyStress_app.properties' with following property 'jvmflags=-Xmx64M' to guarantee the test fails on all machines.

          Successfully ran derbyall with Sun JDK 1.4.2 on Windows XP. Please take a look at this patch.

          Show
          Deepa Remesh added a comment - I am uploading a combined patch 'derby-210.diff' which solves the memory leak. As Bryan suggested, I am uploading this patch and will open jira issues for other optimizations. Patch does the following: Eliminates the below references to PreparedStatement objects by using WeakHashMap instead of LinkedList. When there are no other references to the keys in a WeakHashMap, they will get removed from the map and can thus get garbage-collected. They do not have to wait till the Connection object is collected. 'openStatements_' in org.apache.derby.client.am.Connection 'CommitAndRollbackListeners_' in org.apache.derby.client.am.Connection Removes the list 'RollbackOnlyListeners_' since this is not being used. Updates the following comment for openStatements_: // Since DERBY prepared statements must be re-prepared after a commit, // then we must traverse this list after a commit and notify statements // that they are now in an un-prepared state. final java.util.LinkedList openStatements_ = new java.util.LinkedList(); In the code, I did not see this list being traversed after a commit to re-prepare statements. Also, I think this is not needed since Derby does not require re-prepare of statements after a commit. Currently, this list is used to close all open statements when the originating connection is closed. Removes all ResultSets from HashTable 'positionedUpdateCursorNameToResultSet_' in SectionManager. Only result sets of positioned update statements were being removed from this hashtable whereas all result sets were added. Because of this, client driver was holding on to result sets and statements even after rs.close() was called. Adds a test 'derbyStress.java' to jdbcapi suite. This test is based on the repro for this patch. Without this patch, it fails when run with client driver. Kathey had suggested in another mail that tests for client memory leak problems ( DERBY-557 , DERBY-210 ) can be added to same test. I did not see an existing test. So I created this new test. If DERBY-557 does not have a test, I think it can be added to this new test. Excludes the new test from running with jcc because jcc gives out of memory error. Creates 'derbyStress_app.properties' with following property 'jvmflags=-Xmx64M' to guarantee the test fails on all machines. Successfully ran derbyall with Sun JDK 1.4.2 on Windows XP. Please take a look at this patch.
          Hide
          Bryan Pendleton added a comment -

          If I understand your comment correctly, you feel that you now have a correct fix, which avoids the memory leaks; however, you are concerned that there is a performance problem because the tracking of the ResultSet objects in the SectionManager hashtable is in some or all cases unnecessary.

          In that case, my opinion is that you should commit the correctness fix now, and file the performance issue as a separate bug and address it separately.

          I will try to take a closer look at your changes over the next week and I will let you know if I have any other comments.

          Show
          Bryan Pendleton added a comment - If I understand your comment correctly, you feel that you now have a correct fix, which avoids the memory leaks; however, you are concerned that there is a performance problem because the tracking of the ResultSet objects in the SectionManager hashtable is in some or all cases unnecessary. In that case, my opinion is that you should commit the correctness fix now, and file the performance issue as a separate bug and address it separately. I will try to take a closer look at your changes over the next week and I will let you know if I have any other comments.
          Hide
          Deepa Remesh added a comment -

          I don't have a patch ready for the rest of the problem. But I have narrowed it down to this:

          The only other reference holding on to the PreparedStatement is in the ResultSet object which remains alive till the PreparedStatement is closed. ResultSet does not get garbage-collected even after ResultSet.close() is called. It gets ready for grabage-collection only when the statement which created it is closed. This is because client driver holds references to ResultSets in a Hashtable 'positionedUpdateCursorNameToResultSet_' in SectionManager object. All ResultSets get added to this Hashtable though only ResultSets from positioned update statements are retrieved and used from this table. Also, only ResultSets of positioned update statements get removed from the HashTable. Other ResultSets remain in the Hashtable. So ResultSet objects cannot get finalized even after ResultSet.close() is called. Because ResultSet has reference to the PreparedStatement object, it also does not get garbage-collected.

          However, when PreparedStatement.close() is called, the section used by the statement is freed and gets re-used by new statements. Server cursor names are associated with a section and so the new statement uses the same server cursor name as previous statement (which has been closed). And ResultSet from new statement gets added to the HashTable with same cursor name as used by previous statement. So the Hashtable value gets over-written and previous ResultSet is now free for garbage collection. And the previous PreparedStatement is also free.

          To verify this, I added following line to the method Statement.resetCursorNameAndRemoveFromWhereCurrentOfMappings

          //Also remove resultset mapping for other cursors
          agent_.sectionManager_.removeCursorNameToResultSetMapping(cursorName_,
          section_.getServerCursorName());

          It was only doing this:
          agent_.sectionManager_.removeCursorNameToResultSetMapping(cursorName_,
          section_.getServerCursorNameForPositionedUpdate());

          With this change and changes from derby-210-patch1.diff, prepared statements get garbage collected even if the user does not explicitly close them. I could run the attached repro derbyStress.java with 40000 prepared statements. I also successfully ran derbynetclientmats.

          Adding the above line seems to solve the problem. But there seems to be some extra work going on in client driver in just adding and removing "all" result sets to the Hashtable 'positionedUpdateCursorNameToResultSet_'. I think this can be avoided. I am trying to understand why positionedUpdateCursorNameToResultSet_ is needed. In the meantime, I'd appreciate any feedback to know that I am in right direction.

          Show
          Deepa Remesh added a comment - I don't have a patch ready for the rest of the problem. But I have narrowed it down to this: The only other reference holding on to the PreparedStatement is in the ResultSet object which remains alive till the PreparedStatement is closed. ResultSet does not get garbage-collected even after ResultSet.close() is called. It gets ready for grabage-collection only when the statement which created it is closed. This is because client driver holds references to ResultSets in a Hashtable 'positionedUpdateCursorNameToResultSet_' in SectionManager object. All ResultSets get added to this Hashtable though only ResultSets from positioned update statements are retrieved and used from this table. Also, only ResultSets of positioned update statements get removed from the HashTable. Other ResultSets remain in the Hashtable. So ResultSet objects cannot get finalized even after ResultSet.close() is called. Because ResultSet has reference to the PreparedStatement object, it also does not get garbage-collected. However, when PreparedStatement.close() is called, the section used by the statement is freed and gets re-used by new statements. Server cursor names are associated with a section and so the new statement uses the same server cursor name as previous statement (which has been closed). And ResultSet from new statement gets added to the HashTable with same cursor name as used by previous statement. So the Hashtable value gets over-written and previous ResultSet is now free for garbage collection. And the previous PreparedStatement is also free. To verify this, I added following line to the method Statement.resetCursorNameAndRemoveFromWhereCurrentOfMappings //Also remove resultset mapping for other cursors agent_.sectionManager_.removeCursorNameToResultSetMapping(cursorName_, section_.getServerCursorName()); It was only doing this: agent_.sectionManager_.removeCursorNameToResultSetMapping(cursorName_, section_.getServerCursorNameForPositionedUpdate()); With this change and changes from derby-210-patch1.diff, prepared statements get garbage collected even if the user does not explicitly close them. I could run the attached repro derbyStress.java with 40000 prepared statements. I also successfully ran derbynetclientmats. Adding the above line seems to solve the problem. But there seems to be some extra work going on in client driver in just adding and removing "all" result sets to the Hashtable 'positionedUpdateCursorNameToResultSet_'. I think this can be avoided. I am trying to understand why positionedUpdateCursorNameToResultSet_ is needed. In the meantime, I'd appreciate any feedback to know that I am in right direction.
          Hide
          Deepa Remesh added a comment -

          I have attached a partial patch 'derby-210-patch1.diff' for this problem. It does not solve the entire memory leak problem.

          PreparedStatement references are stored by client driver at following places:
          1. In org.apache.derby.client.am.Connection, they are added to java.util.LinkedList 'openStatements_'
          2. In org.apache.derby.client.am.Connection, they are added to java.util.LinkedList 'CommitAndRollbackListeners_'

          This patch eliminates the above references by using WeakHashMap instead of LinkedList. When there are no other references to the keys in a WeakHashMap, they will get removed from the map and can thus get garbage-collected. They do not have to wait till the Connection object is collected.

          I removed the list RollbackOnlyListeners_ since this is not being used.

          I have also updated the following comment for openStatements_:
          // Since DERBY prepared statements must be re-prepared after a commit,
          // then we must traverse this list after a commit and notify statements
          // that they are now in an un-prepared state.
          final java.util.LinkedList openStatements_ = new java.util.LinkedList();

          In the code, I did not see this list being traversed after a commit to re-prepare statements. Also, I think this is not needed since Derby does not require re-prepare of statements after a commit. Currently, this list is used to close all open statements when the originating connection is closed.

          With this patch, I ran derbyall successfully with Sun JDK 1.4.2 on Windows XP. I'd appreciate if someone can review this patch and provide me feedback.

          Show
          Deepa Remesh added a comment - I have attached a partial patch 'derby-210-patch1.diff' for this problem. It does not solve the entire memory leak problem. PreparedStatement references are stored by client driver at following places: 1. In org.apache.derby.client.am.Connection, they are added to java.util.LinkedList 'openStatements_' 2. In org.apache.derby.client.am.Connection, they are added to java.util.LinkedList 'CommitAndRollbackListeners_' This patch eliminates the above references by using WeakHashMap instead of LinkedList. When there are no other references to the keys in a WeakHashMap, they will get removed from the map and can thus get garbage-collected. They do not have to wait till the Connection object is collected. I removed the list RollbackOnlyListeners_ since this is not being used. I have also updated the following comment for openStatements_: // Since DERBY prepared statements must be re-prepared after a commit, // then we must traverse this list after a commit and notify statements // that they are now in an un-prepared state. final java.util.LinkedList openStatements_ = new java.util.LinkedList(); In the code, I did not see this list being traversed after a commit to re-prepare statements. Also, I think this is not needed since Derby does not require re-prepare of statements after a commit. Currently, this list is used to close all open statements when the originating connection is closed. With this patch, I ran derbyall successfully with Sun JDK 1.4.2 on Windows XP. I'd appreciate if someone can review this patch and provide me feedback.
          Hide
          Kathey Marsden added a comment -

          The workaround for this issue is for the application to explicitly close all prepared statements. The close is often omitted for error conditions causing slow leaks. Applications can't rely on garbage collection to clean up prepared statements.

          A good way to diagnose which statements are leaking if you have this
          problem is to run
          java org.apache.derby.drda.NetworkServerControl runtimeinfo

          runtimeinfo shows the statements currently open on the server.

          Show
          Kathey Marsden added a comment - The workaround for this issue is for the application to explicitly close all prepared statements. The close is often omitted for error conditions causing slow leaks. Applications can't rely on garbage collection to clean up prepared statements. A good way to diagnose which statements are leaking if you have this problem is to run java org.apache.derby.drda.NetworkServerControl runtimeinfo runtimeinfo shows the statements currently open on the server.
          Hide
          Kathey Marsden added a comment -

          This is actually a client issue. There is no protocol in the specification to close a prepared statement, but if the client, reuses section numbers in the PKGNAMCSN for prepared statements that are no longer referenced by the user, this leak could be mitigated. Maybe WeakReference could be utilized for this purpose.

          Show
          Kathey Marsden added a comment - This is actually a client issue. There is no protocol in the specification to close a prepared statement, but if the client, reuses section numbers in the PKGNAMCSN for prepared statements that are no longer referenced by the user, this leak could be mitigated. Maybe WeakReference could be utilized for this purpose.

            People

            • Assignee:
              Unassigned
              Reporter:
              Kathey Marsden
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Development