db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Deepa Remesh (JIRA)" <derby-...@db.apache.org>
Subject [jira] Updated: (DERBY-210) Network Server will leak prepared statements if not explicitly closed by the user until the connection is closed
Date Thu, 09 Feb 2006 18:35:59 GMT
     [ http://issues.apache.org/jira/browse/DERBY-210?page=all ]

Deepa Remesh updated DERBY-210:
-------------------------------

    Attachment: derby-210-v2-draft.diff
                derby-210-v2-draft.status

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

> Network Server will leak prepared statements if not explicitly closed by the user until
the connection is closed
> ----------------------------------------------------------------------------------------------------------------
>
>          Key: DERBY-210
>          URL: http://issues.apache.org/jira/browse/DERBY-210
>      Project: Derby
>         Type: Bug
>   Components: Network Client
>     Reporter: Kathey Marsden
>     Assignee: Deepa Remesh
>  Attachments: DOTS_ATCJ2_Derby-noPatch.png, DOTS_ATCJ2_Derby-withPatch.png, derby-210-v2-draft.diff,
derby-210-v2-draft.status, derby-210.diff, derby-210.status, derbyStress.java
>
> 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.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


Mime
View raw message