Client driver gets OutOfMemoryError when re-executing statement without closing ResultSet

• Key: DERBY-557
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Knut Anders Hatlen
• Reporter: Knut Anders Hatlen
• Component/s: Network Client
• Affects Version/s: 10.1.1.0, 10.1.2.1, 10.2.1.6
• Fix Version/s: 10.1.2.1, 10.2.1.6

Description

When re-executing a statement without closing the old ResultSet, some of the old ResultSet's resources are not released, and the network client will eventually get an OutOfMemoryError.

Example:

Connection c = DriverManager.getConnection("jdbc:derby://localhost/mydb");
PreparedStatement ps = c.prepareStatement("select * from sys.systables");
while (true) {
ResultSet rs = ps.executeQuery();
}

This code will run for some time and then throw an OutOfMemoryError. Same thing happens with Statement instead of PreparedStatement. If rs.close() is added in the loop, the code works. Explicitly closing the ResultSet should not be necessary according to this quote from the JDBC 3.0 spec:

  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

Comments

[Knut Anders Hatlen]

Quoting myself from derby-user:

> FYI, I had a look in the client driver code. It seems like only
> markClosed() is called on the open ResultSets when a statement is
> re-executed. The difference between close() and markClosed() is
> basically that close() additionally checks whether it should
> auto-commit, nulls out the cursor and meta data, and calls
>
> connection_.CommitAndRollbackListeners_.remove(this);
>
> Could this be our memory leak? Is there any situation where one
> shouldn't call close() on the result sets when re-executing?

After I sent this mail, I ran a test with some println-statements in the client driver. When the OutOfMemoryError was thrown the CommitAndRollbackListeners_ list had around 17000 elements when using Statement, and around 300000 elements when using PreparedStatement.

[Knut Anders Hatlen]

Attached a patch which fixes the bug. The patch ensures that the ResultSets associated with a Statement/PreparedStatement are removed from CommitAndRollbackListeners_ in org.apache.derby.client.am.Connection when the statement is re-executed.

I have run derbyall with only one error (wrong length of encryption key, not related to the patch). I have also run the program in the problem description (both with Statement and PreparedStatement), and the memory usage doesn't increase over time.

Could someone review this patch?

Thanks!

[Kathey Marsden]

Thanks Knut for the patch.
It looks good and I committed it to the trunk.

Date: Fri Sep 16 06:52:14 2005
New Revision: 289539

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


A couple of questions though.

1) What do you think about adding a test.? The heap can be set using jvmflags in an app_properties file as we do in tests/largedata

2) Do you want this to go into the 10.1.2 release? If so, you can just merge and test in your workspace and then post the svn merge command here and I'll port it to the branch. I think it would be a great to address some of these memory issues for 10.1.2.

[Knut Anders Hatlen]

Kathey,

Thanks for committing. Answers to your questions follow.

1) I could write a test like "run this loop for x minutes and see if an OutOfMemoryError is thrown". With a small heap it would run out of memory fairly soon. Is this the kind of test you want? Where does such a test belong? derbylang? derby*mats?

2) I merged the changes and tested with 10.1 earlier today. Didn't get OutOfMemoryError and derbyall ran successfully (except some encryption tests, as usual). Merge command: svn merge -r 289538:289539 trunk-directory 10.1-directory

[Kathey Marsden]

That sounds fine to me for the test or just some large constant number of iterations.

If you are adding a new test, I tend to think jdbcapi is a good place and something with a name generic enough that we could put in a test for DERBY-210. Alternately you could just change resultset.java to use a small heap if this doesn't add a huge amount of time to the test.

[Kathey Marsden]

Should I go ahead and port this to 10.1 or wait for the test?

[Knut Anders Hatlen]

Kathey, I think you should just go ahead and port the fix to 10.1.

Merge command:

  svn merge -r 289538:289539 https://svn.apache.org/repos/asf/db/derby/code/trunk

I have started working on a general check-that-resources-are-freed
test for DERBY-23, DERBY-210 and DERBY-557, but while doing that I ran
into another resource leak (DERBY-594) that I wanted to resolve first.

[Knut Anders Hatlen]

Fixed in trunk (revision 289539) and 10.1 (revision 306950).

[Knut Anders Hatlen]

Reopening to add regression test.

[Knut Anders Hatlen]

Added a test case for DERBY-557 to jdbcapi/derbyStress.java. I have
verified that the test case fails without the fix.

I had to enable the test for DerbyNetClient. Since the test case for
DERBY-210 currently fails under DerbyNetClient, I disabled that test
case in the java code by adding this to prepStmtTest():

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

Derbyall ran cleanly.

Could someone please review?

Thanks.

[Bryan Pendleton]

This change looks good to me. The patch applies cleanly to the current trunk. I read through the test code and thought it was fine. With your patch applied, I ran

java -Dframework=DerbyNetClient -Dverbose=true org.apache.derbyTesting.functionTests.harness.RunTest jdbcapi/derbyStress.java

and got the expected results (test passed, with the new print statement appearing in the output as expected).

[Knut Anders Hatlen]

Whops! I forgot to close the connection in the regression test. Uploaded new patch (DERBY-557-regression-test-v2.diff) with conn.close() added. I apologize the noise.

[Knut Anders Hatlen]

Uploading new diff because of conflict with a recent commit.

[Bernt M. Johnsen]

Committed revision 377998.

[Knut Anders Hatlen]

Regression test checked into revision 377998.