Derby
  1. Derby
  2. DERBY-3316

Leak in client if ResultSet not closed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6, 10.3.2.1, 10.4.1.3
    • Fix Version/s: 10.3.3.0, 10.4.1.3
    • Component/s: Network Client
    • Labels:
      None
    • Bug behavior facts:
      Regression

      Description

      If I run the attached program RepeatStatement.java with 32M of heap,
      I will get an OutOfMemory error in the client.

      java -Xmx32M RepeatStatement
      Exception in thread "main" java.lang.OutOfMemoryError: Java heap space
      at org.apache.derby.client.am.Cursor.allocateCharBuffer(Cursor.java:1260)
      at org.apache.derby.client.net.NetStatementReply.parseSQLDTARDarray(NetStatementReply.java:1356)
      at org.apache.derby.client.net.NetStatementReply.parseQRYDSC(NetStatementReply.java:1207)
      at org.apache.derby.client.net.NetStatementReply.parseOpenQuery(NetStatementReply.java:479)
      at org.apache.derby.client.net.NetStatementReply.parseOPNQRYreply(NetStatementReply.java:223)
      at org.apache.derby.client.net.NetStatementReply.readOpenQuery(NetStatementReply.java:64)
      at org.apache.derby.client.net.StatementReply.readOpenQuery(StatementReply.java:50)
      at org.apache.derby.client.net.NetStatement.readOpenQuery_(NetStatement.java:153)
      at org.apache.derby.client.am.Statement.readOpenQuery(Statement.java:1396)
      at org.apache.derby.client.am.Statement.flowExecute(Statement.java:2001)
      at org.apache.derby.client.am.Statement.executeQueryX(Statement.java:421)
      at org.apache.derby.client.am.Statement.executeQuery(Statement.java:406)
      at RepeatStatement.testInsertAndSelect(RepeatStatement.java:31)
      at RepeatStatement.main(RepeatStatement.java:10)

      If I close the ResultSet or Statement it does not leak.

      This occurs on trunk and 10.2.1.6. It does however not run out of memory on 10.1.3.1, so appears to be a regression.

      1. RepeatStatement.java
        1 kB
        Kathey Marsden
      2. move_derbystress_to_derbyall_diff.txt
        1 kB
        Kathey Marsden
      3. derby-3316_diff2.txt
        4 kB
        Kathey Marsden
      4. derby-3316_diff.txt
        4 kB
        Kathey Marsden

        Issue Links

          Activity

          Hide
          Kathey Marsden added a comment -

          The leak appears to be in SectionManager.positionedUpdateCursorNameToQuerySection_ which contrary to its name holds cursor mapping for all cursors, not just positioned updates. These are not getting cleaned up in finalization.

          Show
          Kathey Marsden added a comment - The leak appears to be in SectionManager.positionedUpdateCursorNameToQuerySection_ which contrary to its name holds cursor mapping for all cursors, not just positioned updates. These are not getting cleaned up in finalization.
          Hide
          Kathey Marsden added a comment -

          The leak was actually in SectionManager.positionedUpdateCursorNameToResultSet_ which kept a reference to the ResultSet so it wouldn't get garbage collected. The solution was to use a WeakReference to the ResultSets in positionedUpdateCursorNameToResultSet_ so that they can be garbage collected. See C:\kmarsden\patches\derby-3316_diff.txt . Running tests now.

          Show
          Kathey Marsden added a comment - The leak was actually in SectionManager.positionedUpdateCursorNameToResultSet_ which kept a reference to the ResultSet so it wouldn't get garbage collected. The solution was to use a WeakReference to the ResultSets in positionedUpdateCursorNameToResultSet_ so that they can be garbage collected. See C:\kmarsden\patches\derby-3316_diff.txt . Running tests now.
          Hide
          Kathey Marsden added a comment -

          There may be a problem with the patch, My suites.All run ran out of memory.

          Show
          Kathey Marsden added a comment - There may be a problem with the patch, My suites.All run ran out of memory.
          Hide
          Knut Anders Hatlen added a comment -

          Doesn't the Hashtable still leak with the patch? It allows the ResultSet objects to be freed, but the Hashtable doesn't remove the empty WeakReference objects, so it will still grow, I think.

          It seems like you planned to add a comment to mapCursorNameToResultSet() but forgot to finish it.

          Show
          Knut Anders Hatlen added a comment - Doesn't the Hashtable still leak with the patch? It allows the ResultSet objects to be freed, but the Hashtable doesn't remove the empty WeakReference objects, so it will still grow, I think. It seems like you planned to add a comment to mapCursorNameToResultSet() but forgot to finish it.
          Hide
          Kathey Marsden added a comment -

          Thank you Knut for looking at the patch. I verified that the entries get removed from the Hashtable as part of the markClosed processing. I printed out the size after calls to removeCursorNameToResultSetMapping and verified that the Hashtable is not growing. I fixed up the comment in the revised patch derby-3316_diff2.txt

          Show
          Kathey Marsden added a comment - Thank you Knut for looking at the patch. I verified that the entries get removed from the Hashtable as part of the markClosed processing. I printed out the size after calls to removeCursorNameToResultSetMapping and verified that the Hashtable is not growing. I fixed up the comment in the revised patch derby-3316_diff2.txt
          Hide
          Daniel John Debrunner added a comment -

          Seems like that the Hashtable positionedUpdateCursorNameToResultSet should really be a java.util.WeakHashMap, this would simplify the code, not exposing WeakReference into get/puts.

          Show
          Daniel John Debrunner added a comment - Seems like that the Hashtable positionedUpdateCursorNameToResultSet should really be a java.util.WeakHashMap, this would simplify the code, not exposing WeakReference into get/puts.
          Hide
          Kathey Marsden added a comment -

          Actually making it a WeakHashMap is what I tried first but a WeakHashMap only creates WeakReferences based on the keys not the values, so that did not work. See the implementation notes at: http://java.sun.com/javase/6/docs/api/java/util/WeakHashMap.html

          Show
          Kathey Marsden added a comment - Actually making it a WeakHashMap is what I tried first but a WeakHashMap only creates WeakReferences based on the keys not the values, so that did not work. See the implementation notes at: http://java.sun.com/javase/6/docs/api/java/util/WeakHashMap.html
          Hide
          Daniel John Debrunner added a comment -

          Ignore that, I don't think a WeakHashMap is suitable, as it puts the WeakReference on the key.

          Show
          Daniel John Debrunner added a comment - Ignore that, I don't think a WeakHashMap is suitable, as it puts the WeakReference on the key.
          Hide
          Knut Anders Hatlen added a comment -

          > I verified that the entries get removed from the Hashtable as part of the markClosed processing.

          Thanks Kathey, that makes sense. +1 to commit.

          Show
          Knut Anders Hatlen added a comment - > I verified that the entries get removed from the Hashtable as part of the markClosed processing. Thanks Kathey, that makes sense. +1 to commit.
          Hide
          Jørgen Løland added a comment -

          r612262 | kmarsden | 2008-01-15 23:36:38 +0100 (Tue, 15 Jan 2008) | 5 lines

          Could this commit have made org.apache.derbyTesting.functionTests.tests.jdbcapi.JDBCHarnessJavaTest fail:

          From suites all:

          There was 1 failure:
          1) derbyStress(org.apache.derbyTesting.functionTests.tests.jdbcapi.JDBCHarnessJa vaTest)junit.framework.ComparisonFailure: Output at line 6 expected:<Test derbyS tress finished.> but was:<FAIL – unexpected exception ****************>
          at org.apache.derbyTesting.functionTests.util.CanonTestCase.compareCanon (CanonTestCase.java:100)
          at org.apache.derbyTesting.functionTests.util.HarnessJavaTest.runTest(Ha rnessJavaTest.java:91)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java: 96)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
          at junit.extensions.TestSetup.run(TestSetup.java:23)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
          at junit.extensions.TestSetup.run(TestSetup.java:23)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57 )
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
          at junit.extensions.TestSetup.run(TestSetup.java:23)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57 )

          Show
          Jørgen Løland added a comment - r612262 | kmarsden | 2008-01-15 23:36:38 +0100 (Tue, 15 Jan 2008) | 5 lines Could this commit have made org.apache.derbyTesting.functionTests.tests.jdbcapi.JDBCHarnessJavaTest fail: From suites all: There was 1 failure: 1) derbyStress(org.apache.derbyTesting.functionTests.tests.jdbcapi.JDBCHarnessJa vaTest)junit.framework.ComparisonFailure: Output at line 6 expected:<Test derbyS tress finished.> but was:<FAIL – unexpected exception ****************> at org.apache.derbyTesting.functionTests.util.CanonTestCase.compareCanon (CanonTestCase.java:100) at org.apache.derbyTesting.functionTests.util.HarnessJavaTest.runTest(Ha rnessJavaTest.java:91) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java: 96) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) at junit.extensions.TestSetup$1.protect(TestSetup.java:19) at junit.extensions.TestSetup.run(TestSetup.java:23) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) at junit.extensions.TestSetup$1.protect(TestSetup.java:19) at junit.extensions.TestSetup.run(TestSetup.java:23) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57 ) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) at junit.extensions.TestSetup$1.protect(TestSetup.java:19) at junit.extensions.TestSetup.run(TestSetup.java:23) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57 )
          Hide
          Kathey Marsden added a comment -

          org.apache.derbyTesting.functionTests.tests.jdbcapi.JDBCHarnessJavaTest is passing for me and I don't see a failure in the tinderbox. What is the output that you see for the test? Perhaps I can glean something from that.

          That said I wonder if derbyStress should be run as part of JDBCHarnessJavaTest since it is supposed to run with -Xmx64M. It seems instead that it should remain in derbyall, since we can't control the heap for JUnit tests. Thoughts?

          Show
          Kathey Marsden added a comment - org.apache.derbyTesting.functionTests.tests.jdbcapi.JDBCHarnessJavaTest is passing for me and I don't see a failure in the tinderbox. What is the output that you see for the test? Perhaps I can glean something from that. That said I wonder if derbyStress should be run as part of JDBCHarnessJavaTest since it is supposed to run with -Xmx64M. It seems instead that it should remain in derbyall, since we can't control the heap for JUnit tests. Thoughts?
          Hide
          Jørgen Løland added a comment -

          Hi Kathey,

          I reran the tests without getting this error - seems like an intermittent test failure. Sorry for the noise.

          Show
          Jørgen Løland added a comment - Hi Kathey, I reran the tests without getting this error - seems like an intermittent test failure. Sorry for the noise.
          Hide
          Kathey Marsden added a comment -

          Attached is a patch to move derbyStress back to derbyall so that we can run it with 64MB heap. I will run tests tonight and commit tomorrow.

          Show
          Kathey Marsden added a comment - Attached is a patch to move derbyStress back to derbyall so that we can run it with 64MB heap. I will run tests tonight and commit tomorrow.
          Hide
          Daniel John Debrunner added a comment -

          An alternative is to add a target under ant that runs the junit test with 64mb rather than reverting back to the harness, which we are trying to get rid of.
          Then it could become part of the ant junit-all target that runs all the junit tests (which is actually more that suites.All).

          Show
          Daniel John Debrunner added a comment - An alternative is to add a target under ant that runs the junit test with 64mb rather than reverting back to the harness, which we are trying to get rid of. Then it could become part of the ant junit-all target that runs all the junit tests (which is actually more that suites.All).
          Hide
          Kathey Marsden added a comment -

          For the adding it to the junit-all target option I think a few things have to happen for it to be run as part of the nightlies.

          1) Convert derbyStress.java to junit.
          2) create a junit-lomem target and make it part of junit-all.
          3) Fix junit-all so that it can run with the nightlies. DERBY-2045 + distribute ant to the testing machines make nightly script changes etc.

          I don't have time right now to pursue all of these, so am proposing we move derbyStress.java back to derbyall for now until these goals can be accomplished so at least the nightlies are testing the low memory scenarios. Right now the test doesn't test much because it won't fail even if there is a leak. I think it was moved to JDBCHarnessJavaTest prematurely. I'd like to move it back to derbyall and open an issue to covert derbyStress outlining the issues. Does that sound ok?

          Show
          Kathey Marsden added a comment - For the adding it to the junit-all target option I think a few things have to happen for it to be run as part of the nightlies. 1) Convert derbyStress.java to junit. 2) create a junit-lomem target and make it part of junit-all. 3) Fix junit-all so that it can run with the nightlies. DERBY-2045 + distribute ant to the testing machines make nightly script changes etc. I don't have time right now to pursue all of these, so am proposing we move derbyStress.java back to derbyall for now until these goals can be accomplished so at least the nightlies are testing the low memory scenarios. Right now the test doesn't test much because it won't fail even if there is a leak. I think it was moved to JDBCHarnessJavaTest prematurely. I'd like to move it back to derbyall and open an issue to covert derbyStress outlining the issues. Does that sound ok?
          Hide
          Daniel John Debrunner added a comment -

          Fine to move the test back, I'd assumed it had been converted to Junit.

          Show
          Daniel John Debrunner added a comment - Fine to move the test back, I'd assumed it had been converted to Junit.

            People

            • Assignee:
              Kathey Marsden
              Reporter:
              Kathey Marsden
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development