fix: avoid reusing closed statements

src/main/java/org/apache/ibatis/executor/ReuseExecutor.java

@@ -1,116 +1,116 @@
-/*
- *    Copyright 2009-2023 the original author or authors.
- *
- *    Licensed under the Apache License, Version 2.0 (the "License");
- *    you may not use this file except in compliance with the License.
- *    You may obtain a copy of the License at
- *
- *       https://www.apache.org/licenses/LICENSE-2.0
- *
- *    Unless required by applicable law or agreed to in writing, software
- *    distributed under the License is distributed on an "AS IS" BASIS,
- *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- *    See the License for the specific language governing permissions and
- *    limitations under the License.
- */
-package org.apache.ibatis.executor;
-
-import java.sql.Connection;
-import java.sql.SQLException;
-import java.sql.Statement;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
-import org.apache.ibatis.cursor.Cursor;
-import org.apache.ibatis.executor.statement.StatementHandler;
-import org.apache.ibatis.logging.Log;
-import org.apache.ibatis.mapping.BoundSql;
-import org.apache.ibatis.mapping.MappedStatement;
-import org.apache.ibatis.session.Configuration;
-import org.apache.ibatis.session.ResultHandler;
-import org.apache.ibatis.session.RowBounds;
-import org.apache.ibatis.transaction.Transaction;
-
-/**
- * @author Clinton Begin
- */
-public class ReuseExecutor extends BaseExecutor {
-
-  private final Map<String, Statement> statementMap = new HashMap<>();
-
-  public ReuseExecutor(Configuration configuration, Transaction transaction) {
-    super(configuration, transaction);
-  }
-
-  @Override
-  public int doUpdate(MappedStatement ms, Object parameter) throws SQLException {
-    Configuration configuration = ms.getConfiguration();
-    StatementHandler handler = configuration.newStatementHandler(this, ms, parameter, RowBounds.DEFAULT, null, null);
-    Statement stmt = prepareStatement(handler, ms.getStatementLog());
-    return handler.update(stmt);
-  }
-
-  @Override
-  public <E> List<E> doQuery(MappedStatement ms, Object parameter, RowBounds rowBounds, ResultHandler resultHandler,
-      BoundSql boundSql) throws SQLException {
-    Configuration configuration = ms.getConfiguration();
-    StatementHandler handler = configuration.newStatementHandler(wrapper, ms, parameter, rowBounds, resultHandler,
-        boundSql);
-    Statement stmt = prepareStatement(handler, ms.getStatementLog());
-    return handler.query(stmt, resultHandler);
-  }
-
-  @Override
-  protected <E> Cursor<E> doQueryCursor(MappedStatement ms, Object parameter, RowBounds rowBounds, BoundSql boundSql)
-      throws SQLException {
-    Configuration configuration = ms.getConfiguration();
-    StatementHandler handler = configuration.newStatementHandler(wrapper, ms, parameter, rowBounds, null, boundSql);
-    Statement stmt = prepareStatement(handler, ms.getStatementLog());
-    return handler.queryCursor(stmt);
-  }
-
-  @Override
-  public List<BatchResult> doFlushStatements(boolean isRollback) {
-    for (Statement stmt : statementMap.values()) {
-      closeStatement(stmt);
-    }
-    statementMap.clear();
-    return Collections.emptyList();
-  }
-
-  private Statement prepareStatement(StatementHandler handler, Log statementLog) throws SQLException {
-    Statement stmt;
-    BoundSql boundSql = handler.getBoundSql();
-    String sql = boundSql.getSql();
-    if (hasStatementFor(sql)) {
-      stmt = getStatement(sql);
-      applyTransactionTimeout(stmt);
-    } else {
-      Connection connection = getConnection(statementLog);
-      stmt = handler.prepare(connection, transaction.getTimeout());
-      putStatement(sql, stmt);
-    }
-    handler.parameterize(stmt);
-    return stmt;
-  }
-
-  private boolean hasStatementFor(String sql) {
-    try {
-      Statement statement = statementMap.get(sql);
-      return statement != null && !statement.getConnection().isClosed();
-    } catch (SQLException e) {
-      return false;
-    }
-  }
-
-  private Statement getStatement(String s) {
-    return statementMap.get(s);
-  }
-
-  private void putStatement(String sql, Statement stmt) {
-    statementMap.put(sql, stmt);
-  }
-
-}
+/*
+ *    Copyright 2009-2023 the original author or authors.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *       https://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+package org.apache.ibatis.executor;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.ibatis.cursor.Cursor;
+import org.apache.ibatis.executor.statement.StatementHandler;
+import org.apache.ibatis.logging.Log;
+import org.apache.ibatis.mapping.BoundSql;
+import org.apache.ibatis.mapping.MappedStatement;
+import org.apache.ibatis.session.Configuration;
+import org.apache.ibatis.session.ResultHandler;
+import org.apache.ibatis.session.RowBounds;
+import org.apache.ibatis.transaction.Transaction;
+
+/**
+ * @author Clinton Begin
+ */
+public class ReuseExecutor extends BaseExecutor {
+
+  private final Map<String, Statement> statementMap = new HashMap<>();
+
+  public ReuseExecutor(Configuration configuration, Transaction transaction) {
+    super(configuration, transaction);
+  }
+
+  @Override
+  public int doUpdate(MappedStatement ms, Object parameter) throws SQLException {
+    Configuration configuration = ms.getConfiguration();
+    StatementHandler handler = configuration.newStatementHandler(this, ms, parameter, RowBounds.DEFAULT, null, null);
+    Statement stmt = prepareStatement(handler, ms.getStatementLog());
+    return handler.update(stmt);
+  }
+
+  @Override
+  public <E> List<E> doQuery(MappedStatement ms, Object parameter, RowBounds rowBounds, ResultHandler resultHandler,
+      BoundSql boundSql) throws SQLException {
+    Configuration configuration = ms.getConfiguration();
+    StatementHandler handler = configuration.newStatementHandler(wrapper, ms, parameter, rowBounds, resultHandler,
+        boundSql);
+    Statement stmt = prepareStatement(handler, ms.getStatementLog());
+    return handler.query(stmt, resultHandler);
+  }
+
+  @Override
+  protected <E> Cursor<E> doQueryCursor(MappedStatement ms, Object parameter, RowBounds rowBounds, BoundSql boundSql)
+      throws SQLException {
+    Configuration configuration = ms.getConfiguration();
+    StatementHandler handler = configuration.newStatementHandler(wrapper, ms, parameter, rowBounds, null, boundSql);
+    Statement stmt = prepareStatement(handler, ms.getStatementLog());
+    return handler.queryCursor(stmt);
+  }
+
+  @Override
+  public List<BatchResult> doFlushStatements(boolean isRollback) {
+    for (Statement stmt : statementMap.values()) {
+      closeStatement(stmt);
+    }
+    statementMap.clear();
+    return Collections.emptyList();
+  }
+
+  private Statement prepareStatement(StatementHandler handler, Log statementLog) throws SQLException {
+    Statement stmt;
+    BoundSql boundSql = handler.getBoundSql();
+    String sql = boundSql.getSql();
+    if (hasStatementFor(sql)) {
+      stmt = getStatement(sql);
+      applyTransactionTimeout(stmt);
+    } else {
+      Connection connection = getConnection(statementLog);
+      stmt = handler.prepare(connection, transaction.getTimeout());
+      putStatement(sql, stmt);
+    }
+    handler.parameterize(stmt);
+    return stmt;
+  }
+
+  private boolean hasStatementFor(String sql) {
+    try {
+      Statement statement = statementMap.get(sql);
+      return statement != null && !statement.isClosed() && !statement.getConnection().isClosed();
+    } catch (SQLException e) {
+      return false;
+    }
+  }
+
+  private Statement getStatement(String s) {
+    return statementMap.get(s);
+  }
+
+  private void putStatement(String sql, Statement stmt) {
+    statementMap.put(sql, stmt);
+  }
+
+}

src/test/java/org/apache/ibatis/executor/ReuseExecutorTest.java

@@ -16,7 +16,22 @@
 package org.apache.ibatis.executor;
 
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.sql.Connection;
+import java.sql.Statement;
+import java.util.Map;
+
+import org.apache.ibatis.executor.statement.StatementHandler;
+import org.apache.ibatis.logging.Log;
+import org.apache.ibatis.mapping.BoundSql;
 import org.apache.ibatis.transaction.Transaction;
 import org.junit.jupiter.api.Test;
 
@@ -26,6 +41,71 @@ class ReuseExecutorTest extends BaseExecutorTest {
   void dummy() {
   }
 
+  @Test
+  void shouldNotReuseClosedStatement() throws Exception {
+    ReuseExecutor executor = new ReuseExecutor(config, mock(Transaction.class));
+    Connection connection = mock(Connection.class);
+    Statement statement = mock(Statement.class);
+    when(statement.isClosed()).thenReturn(true);
+    when(statement.getConnection()).thenReturn(connection);
+    when(connection.isClosed()).thenReturn(false);
+
+    Field statementMapField = ReuseExecutor.class.getDeclaredField("statementMap");
+    statementMapField.setAccessible(true);
+    @SuppressWarnings("unchecked")
+    Map<String, Statement> statementMap = (Map<String, Statement>) statementMapField.get(executor);
+    statementMap.put("SELECT 1", statement);
+
+    Method hasStatementFor = ReuseExecutor.class.getDeclaredMethod("hasStatementFor", String.class);
+    hasStatementFor.setAccessible(true);
+
+    boolean result = (boolean) hasStatementFor.invoke(executor, "SELECT 1");
+
+    assertFalse(result);
+    verify(statement).isClosed();
+    verify(statement, never()).getConnection();
+  }
+
+  @Test
+  void shouldPrepareNewStatementWhenCachedStatementIsClosed() throws Exception {
+    Transaction transaction = mock(Transaction.class);
+    ReuseExecutor executor = new ReuseExecutor(config, transaction);
+    Connection connection = mock(Connection.class);
+    Log log = mock(Log.class);
+    StatementHandler handler = mock(StatementHandler.class);
+    BoundSql boundSql = mock(BoundSql.class);
+    Statement closedStatement = mock(Statement.class);
+    Statement newStatement = mock(Statement.class);
+
+    when(log.isDebugEnabled()).thenReturn(false);
+    when(transaction.getConnection()).thenReturn(connection);
+    when(transaction.getTimeout()).thenReturn(0);
+    when(handler.getBoundSql()).thenReturn(boundSql);
+    when(boundSql.getSql()).thenReturn("SELECT 1");
+    when(closedStatement.isClosed()).thenReturn(true);
+    when(closedStatement.getConnection()).thenReturn(connection);
+    when(connection.isClosed()).thenReturn(false);
+    when(handler.prepare(connection, 0)).thenReturn(newStatement);
+
+    Field statementMapField = ReuseExecutor.class.getDeclaredField("statementMap");
+    statementMapField.setAccessible(true);
+    @SuppressWarnings("unchecked")
+    Map<String, Statement> statementMap = (Map<String, Statement>) statementMapField.get(executor);
+    statementMap.put("SELECT 1", closedStatement);
+
+    Method prepareStatement = ReuseExecutor.class.getDeclaredMethod("prepareStatement", StatementHandler.class,
+        Log.class);
+    prepareStatement.setAccessible(true);
+
+    Statement result = (Statement) prepareStatement.invoke(executor, handler, log);
+
+    assertSame(newStatement, result);
+    verify(closedStatement).isClosed();
+    verify(closedStatement, never()).getConnection();
+    verify(handler).prepare(connection, 0);
+    verify(handler).parameterize(newStatement);
+  }
+
   @Override
   @Test
   public void shouldFetchPostWithBlogWithCompositeKey() {