You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2013/06/20 23:34:09 UTC

svn commit: r1495197 - /tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java

Author: markt
Date: Thu Jun 20 21:34:09 2013
New Revision: 1495197

URL: http://svn.apache.org/r1495197
Log:
r1495154 incorrectly removed the commit
Restore the commit and clean the code up a little.

Modified:
    tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java

Modified: tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java?rev=1495197&r1=1495196&r2=1495197&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java (original)
+++ tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java Thu Jun 20 21:34:09 2013
@@ -527,47 +527,42 @@ public class JDBCRealm
         // connection may try to be opened again. On normal conditions (including
         // invalid login - the above is only used once.
         int numberOfTries = 2;
-        while (numberOfTries>0) {
+        while (numberOfTries > 0) {
             try {
-
                 // Ensure that we have an open database connection
                 open();
 
-                try {
-                    stmt = credentials(dbConnection, username);
-                    rs = stmt.executeQuery();
+                stmt = credentials(dbConnection, username);
+                rs = stmt.executeQuery();
+                dbConnection.commit();
 
-                    if (rs.next()) {
-                        dbCredentials = rs.getString(1);
-                    }
-                    rs.close();
-                    rs = null;
-                    if (dbCredentials == null) {
-                        return (null);
-                    }
+                if (rs.next()) {
+                    dbCredentials = rs.getString(1);
+                }
 
+                if (dbCredentials != null) {
                     dbCredentials = dbCredentials.trim();
-                    return dbCredentials;
-
-                } finally {
-                    if (rs!=null) {
-                        try {
-                            rs.close();
-                        } catch(SQLException e) {
-                            containerLog.warn(sm.getString("jdbcRealm.abnormalCloseResultSet"));
-                        }
-                    }
                 }
 
-            } catch (SQLException e) {
+                return dbCredentials;
 
+            } catch (SQLException e) {
                 // Log the problem for posterity
                 containerLog.error(sm.getString("jdbcRealm.exception"), e);
+            } finally {
+                if (rs != null) {
+                    try {
+                        rs.close();
+                    } catch(SQLException e) {
+                        containerLog.warn(sm.getString(
+                                "jdbcRealm.abnormalCloseResultSet"));
+                    }
+                }
+            }
 
-                // Close the connection so that it gets reopened next time
-                if (dbConnection != null)
-                    close(dbConnection);
-
+            // Close the connection so that it gets reopened next time
+            if (dbConnection != null) {
+                close(dbConnection);
             }
 
             numberOfTries--;



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Re: svn commit: r1495197 - /tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java

Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,

On 6/20/13 5:34 PM, markt@apache.org wrote:
> Author: markt
> Date: Thu Jun 20 21:34:09 2013
> New Revision: 1495197
> 
> URL: http://svn.apache.org/r1495197
> Log:
> r1495154 incorrectly removed the commit
> Restore the commit and clean the code up a little.
> 
> Modified:
>     tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java
> 
> Modified: tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java?rev=1495197&r1=1495196&r2=1495197&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/realm/JDBCRealm.java Thu Jun 20 21:34:09 2013
> @@ -527,47 +527,42 @@ public class JDBCRealm
>          // connection may try to be opened again. On normal conditions (including
>          // invalid login - the above is only used once.
>          int numberOfTries = 2;
> -        while (numberOfTries>0) {
> +        while (numberOfTries > 0) {
>              try {
> -
>                  // Ensure that we have an open database connection
>                  open();
>  
> -                try {
> -                    stmt = credentials(dbConnection, username);
> -                    rs = stmt.executeQuery();
> +                stmt = credentials(dbConnection, username);
> +                rs = stmt.executeQuery();
> +                dbConnection.commit();

I still don't understand the commit, here.

I read Konstantin's comment about JDBCRealm /not/ running in auto-commit
mode... why is that? I don't see any INSERT/UPDATE/DELETE statements
anywhere in the code, nor are they user-configurable. So, why bother
with conn.setAutoCommit(false) and conn.commit() in the first place?

If we are going to have commit() why not rollback()?

The use of commit() and/or rollback() implies that there is some
connection state which is useful to the thread of execution. This cannot
be the case as JDBCRealm uses a single-connection yet allows multiple
threads to use it.

Any reason not to eliminate all manipulation of the auto-commit state
/and/ remove the commit calls?

-chris