Leak in client if ResultSet not closed

• Key: DERBY-3316
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Kathey Marsden
• Reporter: Kathey Marsden
• Component/s: Network Client
• 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

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.

Comments

[Kathey Marsden]

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.

[Kathey Marsden]

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.

[Kathey Marsden]

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

[Knut Anders Hatlen]

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.

[Kathey Marsden]

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

[Daniel John Debrunner]

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

[Kathey Marsden]

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

[Daniel John Debrunner]

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

[Knut Anders Hatlen]

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

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

[Jørgen Løland]

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 )

[Kathey Marsden]

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?

[Jørgen Løland]

Hi Kathey,

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

[Kathey Marsden]

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.

[Daniel John Debrunner]

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).

[Kathey Marsden]

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?

[Daniel John Debrunner]

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