[misc] correction of possible race condition on statement.close().

mariadb-corporation · Feb 21, 2016 · 161f581 · 161f581
1 parent da00357
commit 161f581
Showing 1 changed file with 55 additions and 52 deletions.
diff --git a/src/main/java/org/mariadb/jdbc/MariaDbStatement.java b/src/main/java/org/mariadb/jdbc/MariaDbStatement.java
@@ -63,6 +63,8 @@ WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWIS
 import java.io.InputStream;
 import java.sql.*;
 import java.util.*;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.regex.Pattern;
 
 
@@ -72,12 +74,14 @@ public class MariaDbStatement implements Statement {
      * the protocol used to talk to the server.
      */
     protected Protocol protocol;
+
     /**
      * the  Connection object.
      */
     protected MariaDbConnection connection;
     protected boolean autoGeneratedKeys;
     protected boolean binaryData = false;
+
     /**
      * The actual query result.
      */
@@ -86,7 +90,7 @@ public class MariaDbStatement implements Statement {
     protected boolean isRewriteable = true;
     protected String firstRewrite = null;
     protected ResultSet batchResultSet = null;
-    boolean isClosed;
+    private AtomicBoolean closed = new AtomicBoolean();
     boolean isTimedout;
     volatile boolean executing;
     List<Query> batchQueries;
@@ -98,6 +102,8 @@ public class MariaDbStatement implements Statement {
     private int fetchSize;
     private boolean isStreaming = false;
     private int maxRows;
+    private ReentrantLock lock;
+
     public static final Pattern deleteEndSemicolonPattern = Pattern.compile("[;][ ]*$", Pattern.CASE_INSENSITIVE);
 
     /**
@@ -111,6 +117,7 @@ public MariaDbStatement(MariaDbConnection connection, int autoGeneratedKeys) {
         this.protocol = connection.getProtocol();
         this.connection = connection;
         this.escapeProcessing = true;
+        this.lock = this.connection.lock;
         cachedResultSets = new LinkedList<>();
     }
 
@@ -140,15 +147,6 @@ public boolean isStreaming() {
         return fetchSize == Integer.MIN_VALUE;
     }
 
-    /**
-     * returns the protocol.
-     *
-     * @return the protocol used.
-     */
-    public Protocol getProtocol() {
-        return protocol;
-    }
-
     // Part of query prolog - setup timeout timer
     private void setTimerTask() {
         assert (timerTask == null);
@@ -166,7 +164,7 @@ public void run() {
     }
 
     void executeQueryProlog() throws SQLException {
-        if (isClosed) {
+        if (closed.get()) {
             throw new SQLException("execute() is called on closed statement");
         }
         if (protocol.isExplicitClosed()) {
@@ -186,7 +184,7 @@ void executeQueryProlog() throws SQLException {
         }
 
         cachedResultSets.clear();
-        ((MariaDbConnection) getConnection()).reenableWarnings();
+        connection.reenableWarnings();
 
         try {
             protocol.setMaxRows(maxRows);
@@ -269,10 +267,9 @@ private void executeQueryEpilog(QueryException queryException, Query query) thro
      * @throws SQLException the error description
      */
     protected boolean execute(Query query) throws SQLException {
-        checkClose();
         executing = true;
         QueryException exception = null;
-        connection.lock.lock();
+        lock.lock();
         try {
             executeQueryProlog();
             try {
@@ -291,7 +288,7 @@ protected boolean execute(Query query) throws SQLException {
                 executing = false;
             }
         } finally {
-            connection.lock.unlock();
+            lock.unlock();
         }
     }
 
@@ -308,11 +305,10 @@ protected boolean execute(Query query) throws SQLException {
      * @throws SQLException the error description
      */
     protected boolean execute(List<Query> queries, boolean isRewritable, int rewriteOffset) throws SQLException {
-        checkClose();
         executing = true;
 
         QueryException exception = null;
-        connection.lock.lock();
+        lock.lock();
         try {
             executeQueryProlog();
             try {
@@ -328,7 +324,7 @@ protected boolean execute(List<Query> queries, boolean isRewritable, int rewrite
                 executing = false;
             }
         } finally {
-            connection.lock.unlock();
+            lock.unlock();
         }
     }
 
@@ -574,30 +570,35 @@ protected Query stringToQuery(String queryString) throws SQLException {
      * @throws java.sql.SQLException if a database access error occurs
      */
     public void close() throws SQLException {
-        if (queryResult != null) {
-            queryResult.close();
-            queryResult = null;
-        }
-        // No possible future use for the cached results, so these can be cleared
-        // This makes the cache eligible for garbage collection earlier if the statement is not
-        // immediately garbage collected
-        cachedResultSets.clear();
-        if (isStreaming()) {
-            connection.lock.lock();
-            try {
-                while (getInternalMoreResults(true)) {
+        closed.set(true);
+        lock.lock();
+        try {
+            if (queryResult != null) {
+                queryResult.close();
+                queryResult = null;
+            }
+            // No possible future use for the cached results, so these can be cleared
+            // This makes the cache eligible for garbage collection earlier if the statement is not
+            // immediately garbage collected
+            cachedResultSets.clear();
+            if (isStreaming()) {
+                lock.lock();
+                try {
+                    while (getInternalMoreResults(true)) {
+                    }
+                } finally {
+                    lock.unlock();
                 }
-            } finally {
-                connection.lock.unlock();
             }
+            protocol = null;
+            if (connection == null || connection.pooledConnection == null
+                    || connection.pooledConnection.statementEventListeners.isEmpty()) {
+                return;
+            }
+            connection.pooledConnection.fireStatementClosed(this);
+        } finally {
+            lock.unlock();
         }
-        protocol = null;
-        isClosed = true;
-        if (connection == null || connection.pooledConnection == null
-                || connection.pooledConnection.statementEventListeners.isEmpty()) {
-            return;
-        }
-        connection.pooledConnection.fireStatementClosed(this);
     }
 
     /**
@@ -840,7 +841,7 @@ public int getResultSetHoldability() throws SQLException {
      * @since 1.6
      */
     public boolean isClosed() throws SQLException {
-        return isClosed;
+        return closed.get();
     }
 
     /**
@@ -904,15 +905,17 @@ protected boolean getInternalMoreResults(boolean streaming) throws SQLException
             if (queryResult != null) {
                 queryResult.close();
             }
+            if (protocol != null) {
+                queryResult = protocol.getMoreResults(streaming);
+                if (queryResult == null) {
+                    return false;
+                }
+                warningsCleared = false;
+                connection.reenableWarnings();
 
-            queryResult = protocol.getMoreResults(streaming);
-            if (queryResult == null) {
-                return false;
+                return queryResult.getResultSetType() == ResultSetType.SELECT;
             }
-            warningsCleared = false;
-            connection.reenableWarnings();
-
-            return queryResult.getResultSetType() == ResultSetType.SELECT;
+            return false;
         } catch (QueryException e) {
             ExceptionMapper.throwException(e, connection, this);
             return false;
@@ -1227,12 +1230,12 @@ public int[] executeBatch() throws SQLException {
         int batchQueriesCount = 0;
         MariaDbResultSet rs = null;
         cachedResultSets.clear();
-        connection.lock.lock();
+        lock.lock();
         try {
-            if (getProtocol().getOptions().allowMultiQueries || getProtocol().getOptions().rewriteBatchedStatements) {
+            if (protocol.getOptions().allowMultiQueries || protocol.getOptions().rewriteBatchedStatements) {
                 int size = batchQueries.size();
                 batchResultSet = null;
-                boolean rewrittenBatch = isRewriteable && getProtocol().getOptions().rewriteBatchedStatements;
+                boolean rewrittenBatch = isRewriteable && protocol.getOptions().rewriteBatchedStatements;
                 execute(batchQueries, rewrittenBatch, (rewrittenBatch && firstRewrite != null) ? firstRewrite.length() : 0);
                 return rewrittenBatch ? getUpdateCountsForReWrittenBatch(size) : getUpdateRewrittenCounts();
             } else {
@@ -1257,7 +1260,7 @@ public int[] executeBatch() throws SQLException {
         } catch (SQLException sqle) {
             throw new BatchUpdateException(sqle.getMessage(), sqle.getSQLState(), sqle.getErrorCode(), Arrays.copyOf(ret, batchQueriesCount), sqle);
         } finally {
-            connection.lock.unlock();
+            lock.unlock();
             clearBatch();
         }
         batchResultSet = rs;
@@ -1418,7 +1421,7 @@ private void checkReconnectWithoutProxy() throws SQLException {
      * @throws SQLException if statement close
      */
     protected void checkClose() throws SQLException {
-        if (isClosed) {
+        if (closed.get()) {
             throw new SQLException("Cannot do an operation on a closed statement");
         }
     }