Mailing-List: contact derby-commits-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Derby Development" <derby-dev@db.apache.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r666088 - in /db/derby/code/trunk/java:
 drda/org/apache/derby/impl/drda/DRDAConnThread.java
 engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java
 engine/org/apache/derby/iapi/jdbc/EngineConnection.java
Date: Tue, 10 Jun 2008 12:46:12 -0000
To: derby-commits@db.apache.org
From: kristwaa@apache.org
Message-Id: <20080610124612.7254623889C1@eris.apache.org>

Author: kristwaa
Date: Tue Jun 10 05:46:11 2008
New Revision: 666088

URL: http://svn.apache.org/viewvc?rev=666088&view=rev
Log:
DERBY-3596: Creation of logical connections from a pooled connection causes resource leak on the server.
Exposed method 'resetFromPool' through EngineConnection.
The network server now detects when a client is requesting new logical connectio
ns. This triggers some special logic, where the physical connection on the serve
r side is kept and reset instead of being closed and opened again (this caused resources to leak earlier).
The special logic must *not* be triggered for XA connections, as the XA code is
already well-behaved.
Patch file: derby-3596-5a-complex_skip_creds.diff

Modified:
    db/derby/code/trunk/java/drda/org/apache/derby/impl/drda/DRDAConnThread.java
    db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java
    db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/EngineConnection.java

Modified: db/derby/code/trunk/java/drda/org/apache/derby/impl/drda/DRDAConnThread.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/drda/org/apache/derby/impl/drda/DRDAConnThread.java?rev=666088&r1=666087&r2=666088&view=diff
==============================================================================
--- db/derby/code/trunk/java/drda/org/apache/derby/impl/drda/DRDAConnThread.java (original)
+++ db/derby/code/trunk/java/drda/org/apache/derby/impl/drda/DRDAConnThread.java Tue Jun 10 05:46:11 2008
@@ -205,6 +205,18 @@
     // to the apparent classloader bug in the JVM.
 	private static final DRDAProtocolExceptionInfo dummy =
 		new DRDAProtocolExceptionInfo(0,0,0,false);
+    /**
+     * Tells if the reset / connect request is a deferred request.
+     * This information is used to work around a bug (DERBY-3596) in a
+     * compatible manner, which also avoids any changes in the client driver.
+     * <p>
+     * The bug manifests itself when a connection pool data source is used and
+     * logical connections are obtained from the physical connection associated
+     * with the data source. Each new logical connection causes a new physical
+     * connection on the server, including a new transaction. These connections
+     * and transactions are not closed / cleaned up.
+     */
+    private boolean deferredReset = false;
 
 	// constructor
 	/**
@@ -1026,7 +1038,12 @@
                 try {
                     PiggyBackedSessionData pbsd =
                             database.getPiggyBackedSessionData(false);
-                    if (pbsd != null) {
+                    // DERBY-3596
+                    // Don't perform this assert if a deferred reset is
+                    // happening or has recently taken place, because the
+                    // connection state has been changed under the feet of the
+                    // piggy-backing mechanism.
+                    if (!this.deferredReset && pbsd != null) {
                         // Session data has already been piggy-backed. Refresh
                         // the data from the connection, to make sure it has
                         // not changed behind our back.
@@ -1470,8 +1487,17 @@
 
 		// If we have already exchanged attributes once just 
 		// process any new manager levels and return (case 2 and 3 above)
+        this.deferredReset = false; // Always reset, only set to true below.
 		if (appRequester != null)
 		{
+            // DERBY-3596
+            // Don't mess with XA requests, as the logic for these are handled
+            // by the server side (embedded) objects. Note that XA requests
+            // results in a different database object implementation, and it
+            // does not have the bug we are working around.
+            if (!appRequester.isXARequester()) {
+                this.deferredReset = true; // Non-XA deferred reset detected.
+            }
 			parseEXCSAT2();
 			return;
 		}
@@ -1890,8 +1916,15 @@
 					else
                     {
                         // reset database for connection re-use 
-                        d.reset();
-						database = d;
+                        // DERBY-3596
+                        // If we are reusing resources for a new physical
+                        // connection, reset the database object. If the client
+                        // is in the process of creating a new logical
+                        // connection only, don't reset the database object.
+                        if (!deferredReset) {
+                            d.reset();
+                        }
+                        database = d;
                     }
 					break;
 				//optional - depending on security Mechanism 
@@ -2665,7 +2698,13 @@
         if (SanityManager.DEBUG) {
             SanityManager.ASSERT(pbsd != null, "pbsd is not expected to be null");
         }
-
+        // DERBY-3596
+        // Reset the flag. In sane builds it is used to avoid an assert, but
+        // we want to reset it as soon as possible to avoid masking real bugs.
+        // We have to do this because we are changing the connection state
+        // at an unexpected time (deferred reset, see parseSECCHK). This was
+        // done to avoid having to change the client code.
+        this.deferredReset = false;
         pbsd.refresh();
         if (pbsd.isModified()) {
             writer.createDssReply();
@@ -2938,6 +2977,28 @@
 		databaseAccessException = null;
 		reader.markCollection();
 		codePoint = reader.getCodePoint();
+        if (this.deferredReset) {
+            // Skip the SECCHK, but assure a minimal degree of correctness.
+            while (codePoint != -1) {
+                switch (codePoint) {
+                    // Note the fall-through.
+                    // Minimal level of checking to detect protocol errors.
+                    // NOTE: SECMGR level 8 code points are not handled.
+                    case CodePoint.SECMGRNM:
+                    case CodePoint.SECMEC:
+                    case CodePoint.SECTKN:
+                    case CodePoint.PASSWORD:
+                    case CodePoint.NEWPASSWORD:
+                    case CodePoint.USRID:
+                    case CodePoint.RDBNAM:
+                        reader.skipBytes();
+                        break;
+                    default:
+                        invalidCodePoint(codePoint);
+                }
+                codePoint = reader.getCodePoint();
+            }
+        } else {
 		while (codePoint != -1)
 		{
 			switch (codePoint)
@@ -3110,11 +3171,29 @@
 		}
 		// RESOLVE - when we do security we need to decrypt encrypted userid & password
 		// before proceeding
+        } // End "if (deferredReset) ... else ..." block
 
 		// verify userid and password, if we haven't had any errors thus far.
 		if ((securityCheckCode == 0) && (databaseAccessException == null))
-		{
-			securityCheckCode = verifyUserIdPassword();
+        {
+            // DERBY-3596: Reset server side (embedded) physical connection for
+            //     use with a new logical connection on the client.
+            if (this.deferredReset) {
+                // Reset the existing connection here.
+                try {
+                    database.getConnection().resetFromPool();
+                    database.getConnection().setHoldability(
+                            ResultSet.HOLD_CURSORS_OVER_COMMIT);
+                    // Reset isolation level to default, as the client is in
+                    // the process of creating a new logical connection.
+                    database.getConnection().setTransactionIsolation(
+                            Connection.TRANSACTION_READ_COMMITTED);
+                } catch (SQLException sqle) {
+                    handleException(sqle);
+                }
+            } else {
+                securityCheckCode = verifyUserIdPassword();
+            }
 		}
 
 		// Security all checked 
@@ -3124,6 +3203,7 @@
 		return securityCheckCode;
 
 	}
+
 	/**
 	 * Write security check reply
 	 * Instance variables

Modified: db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java?rev=666088&r1=666087&r2=666088&view=diff
==============================================================================
--- db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java (original)
+++ db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/BrokeredConnection.java Tue Jun 10 05:46:11 2008
@@ -653,4 +653,12 @@
             throw se;
         }
     }
+
+    /**
+     * @see org.apache.derby.iapi.jdbc.EngineConnection
+     */
+    public void resetFromPool()
+            throws SQLException {
+        getRealConnection().resetFromPool();
+    }
 }

Modified: db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/EngineConnection.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/EngineConnection.java?rev=666088&r1=666087&r2=666088&view=diff
==============================================================================
--- db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/EngineConnection.java (original)
+++ db/derby/code/trunk/java/engine/org/apache/derby/iapi/jdbc/EngineConnection.java Tue Jun 10 05:46:11 2008
@@ -100,4 +100,14 @@
      * @throws java.sql.SQLException
      */
     public String getCurrentSchemaName() throws SQLException;
+
+    /**
+     * Resets the connection before it is returned from a PooledConnection
+     * to a new application request (wrapped by a BrokeredConnection).
+     * <p>
+     * Note that resetting the transaction isolation level is not performed as
+     * part of this method. Temporary tables, IDENTITY_VAL_LOCAL and current
+     * schema are reset.
+     */
+    public void resetFromPool() throws SQLException;
 }