refactored to enforceCleanClose

Author: rPraml

ebean-datasource-api/src/main/java/io/ebean/datasource/DataSourceBuilder.java

@@ -751,25 +751,24 @@ default DataSourceBuilder initDatabaseForPlatform(String platform) {
 
 
   /**
-   * Sets the behaviour what to do, when a connection is closed (=returned to pool), while there is uncommitted work.
+   * When enabled, the datasource enforces a clean close. This means, if you close a possible dirty
+   * connection, that was not committed or rolled back, an exception is thrown.
    * <p>
-   * Possible values
-   * <ul>
-   *   <li><code>nothing</code> nothing happens</li>
-   *   <li><code>rollback</code> a rollback is performed (default)</li>
-   *   <li><code>commit</code> a commit is performed (dangerous!)</li>
-   *   <li><code>remove</code> the connection is removed from the pool (not recommended for production, connection might leak)</li>
-   *   <li><code>fail</code> an exception is thrown by close itself (for debugging, not recommended for production)</li>
-   * </ul>
+   * When disabled, the situation is logged as warning.
+   * <p>
+   * This option has no effect on readonly or autocommit connections.
+   * <p>
+   * Note: It is recommended to enable this option in tests/test systems to find possible
+   * programming errors. See https://github.com/ebean-orm/ebean-datasource/issues/116 for details.
    */
-  DataSourceBuilder closeWithinTxn(String closeWithinTxn);
+  DataSourceBuilder enforceCleanClose(boolean enforceCleanClose);
 
   /**
-   * Returns the behaviour, what to do, when a connection is (=returned to pool), while there is uncommitted work.
+   * When <code>true</code>, an exception is thrown when a dirty connection is closed.
    * <p>
-   * See {@link #closeWithinTxn(String)}.
+   * See {@link #enforceCleanClose(boolean)}.
    */
-  String closeWithinTxn();
+  boolean enforceCleanClose();
 
   /**
    * The settings of the DataSourceBuilder. Provides getters/accessors

ebean-datasource-api/src/main/java/io/ebean/datasource/DataSourceConfig.java

@@ -83,7 +83,7 @@ public class DataSourceConfig implements DataSourceBuilder.Settings {
   private String applicationName;
   private boolean shutdownOnJvmExit;
   private boolean validateOnHeartbeat = !System.getenv().containsKey("LAMBDA_TASK_ROOT");
-  private String closeWithinTxn = "rollback";
+  private boolean enforceCleanClose;
 
   @Override
   public Settings settings() {
@@ -145,7 +145,7 @@ public DataSourceConfig copy() {
     copy.initSql = initSql;
     copy.alert = alert;
     copy.listener = listener;
-    copy.closeWithinTxn = closeWithinTxn;
+    copy.enforceCleanClose = enforceCleanClose;
     return copy;
   }
 
@@ -800,7 +800,7 @@ private void loadSettings(ConfigPropertiesHelper properties) {
     offline = properties.getBoolean("offline", offline);
     shutdownOnJvmExit = properties.getBoolean("shutdownOnJvmExit", shutdownOnJvmExit);
     validateOnHeartbeat = properties.getBoolean("validateOnHeartbeat", validateOnHeartbeat);
-    closeWithinTxn = properties.get("closeWithinTxn", closeWithinTxn);
+    enforceCleanClose = properties.getBoolean("enforceCleanClose", enforceCleanClose);
 
 
     String isoLevel = properties.get("isolationLevel", _isolationLevel(isolationLevel));
@@ -851,14 +851,14 @@ Map<String, String> parseCustom(String customProperties) {
   }
 
   @Override
-  public DataSourceBuilder closeWithinTxn(String closeWithinTxn) {
-    this.closeWithinTxn = closeWithinTxn;
+  public DataSourceBuilder enforceCleanClose(boolean enforceCleanClose) {
+    this.enforceCleanClose = enforceCleanClose;
     return this;
   }
 
   @Override
-  public String closeWithinTxn() {
-    return closeWithinTxn;
+  public boolean enforceCleanClose() {
+    return enforceCleanClose;
   }
 
   /**

ebean-datasource/src/main/java/io/ebean/datasource/pool/ConnectionPool.java

@@ -25,10 +25,6 @@
  */
 final class ConnectionPool implements DataSourcePool {
 
-  enum CloseWithinTxn {
-    NOTHING, ROLLBACK, COMMIT, FAIL, REMOVE;
-  }
-
   private static final String APPLICATION_NAME = "ApplicationName";
   private final ReentrantLock heartbeatLock = new ReentrantLock(false);
   private final ReentrantLock notifyLock = new ReentrantLock(false);
@@ -59,7 +55,7 @@ enum CloseWithinTxn {
   private final boolean failOnStart;
   private final int maxInactiveMillis;
   private final long validateStaleMillis;
-  private final CloseWithinTxn closeWithinTxn;
+  private final boolean enforceCleanClose;
   /**
    * Max age a connection is allowed in millis.
    * A value of 0 means no limit (no trimming based on max age).
@@ -134,7 +130,7 @@ enum CloseWithinTxn {
     this.user = params.getUsername();
     this.shutdownOnJvmExit = params.isShutdownOnJvmExit();
     this.source = DriverDataSource.of(name, params);
-    this.closeWithinTxn = Enum.valueOf(CloseWithinTxn.class, params.closeWithinTxn().toUpperCase(Locale.ROOT));
+    this.enforceCleanClose = params.enforceCleanClose();
     if (!params.isOffline()) {
       init();
     }
@@ -527,32 +523,8 @@ boolean invalidConnection(PooledConnection conn) {
   /**
    * Fail hard in close() when there is uncommitted work. This is for debugging to find wrong code.
    */
-  boolean failIfWithinTransaction() {
-    return closeWithinTxn == CloseWithinTxn.FAIL;
-  }
-
-  /**
-   * will be called, before the connection is returned to the pool. This happens, when there may
-   * be uncommitted changes and before all settings like autocommit/schema/catalog are reset to default.
-   * <p>
-   * If this method fails, the connection is silently removed from pool
-   */
-  void closeWithinTxn(PooledConnection pooledConnection) throws SQLException {
-    switch (closeWithinTxn) {
-      case NOTHING:
-        Log.trace("Closing active connection.");
-        break;
-      case ROLLBACK:
-        pooledConnection.rollback();
-        Log.trace("Closing active connection. Rollback performed.");
-        break;
-      case COMMIT:
-        pooledConnection.commit();
-        Log.trace("Closing active connection. Commit performed.");
-        break;
-      case REMOVE:
-        throw new SQLException("Closing active connection. Removing from pool.");
-    }
+  boolean enforceCleanClose() {
+    return enforceCleanClose;
   }
 
   /**

ebean-datasource/src/main/java/io/ebean/datasource/pool/PooledConnection.java

@@ -219,7 +219,7 @@ String description() {
   }
 
   String fullDescription() {
-    return "name[" + name + "] startTime[" + startUseTime() + "] busySeconds[" + busySeconds() + "] stackTrace[" + stackTraceAsString() + "] stmt[" + lastStatement() + "]";
+    return "name[" + name + "] startTime[" + startUseTime() + "] busySeconds[" + busySeconds() + "] stackTrace[" + stackTraceAsString(stackTrace) + "] stmt[" + lastStatement() + "]";
   }
 
   /**
@@ -449,9 +449,9 @@ public void close() throws SQLException {
       throw new SQLException(IDLE_CONNECTION_ACCESSED_ERROR + "close()");
     }
     boolean mayHaveUncommittedChanges = !autoCommit && !readOnly && status == STATUS_ACTIVE;
-    if (mayHaveUncommittedChanges && pool.failIfWithinTransaction()) {
+    if (mayHaveUncommittedChanges && pool.enforceCleanClose()) {
       pool.returnConnectionForceClose(this);
-      throw new SQLException("Tried to close active connection within transaction");
+      throw new AssertionError("Tried to close a dirty connection. See https://github.com/ebean-orm/ebean-datasource/issues/116 for details.");
     }
     if (hadErrors) {
       if (failoverToReadOnly) {
@@ -469,7 +469,9 @@ public void close() throws SQLException {
         return;
       }
       if (mayHaveUncommittedChanges) {
-        pool.closeWithinTxn(this);
+        Log.warn("Tried to close a dirty connection at {0}. See https://github.com/ebean-orm/ebean-datasource/issues/116 for details.",
+          stackTraceAsString(Thread.currentThread().getStackTrace()));
+        connection.rollback();
       }
       // reset the autoCommit back if client code changed it
       if (autoCommit != pool.isAutoCommit()) {
@@ -949,35 +951,23 @@ void setStackTrace(StackTraceElement[] stackTrace) {
   /**
    * Return the stackTrace as a String for logging purposes.
    */
-  private String stackTraceAsString() {
-    StackTraceElement[] stackTrace = stackTrace();
+  private String stackTraceAsString(StackTraceElement[] stackTrace) {
     if (stackTrace == null) {
       return "";
     }
-    return Arrays.toString(stackTrace);
-  }
-
-  /**
-   * Return the full stack trace that got the connection from the pool. You
-   * could use this if getCreatedByMethod() doesn't work for you.
-   */
-  private StackTraceElement[] stackTrace() {
-    if (stackTrace == null) {
-      return null;
-    }
 
     // filter off the top of the stack that we are not interested in
     ArrayList<StackTraceElement> filteredList = new ArrayList<>();
     boolean include = false;
     for (StackTraceElement stackTraceElement : stackTrace) {
-      if (!include && includeMethodLine(stackTraceElement.toString())) {
+      if (!include && includeMethodLine(stackTraceElement.getClassName())) {
         include = true;
       }
       if (include && filteredList.size() < maxStackTrace) {
         filteredList.add(stackTraceElement);
       }
     }
-    return filteredList.toArray(new StackTraceElement[0]);
+    return filteredList.toString();
   }
 
 }

ebean-datasource/src/test/java/io/ebean/datasource/pool/ConnectionPoolCloseTest.java

@@ -0,0 +1,175 @@
+package io.ebean.datasource.pool;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
+import io.ebean.datasource.DataSourceBuilder;
+import io.ebean.datasource.DataSourcePool;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.LoggerFactory;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.sql.SQLSyntaxErrorException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/**
+ * Tests, if connections are clean when closing them.
+ * <p>
+ * Test must run with "do not use module path" option in IntelliJ.
+ * <p>
+ * See also https://github.com/ebean-orm/ebean-datasource/issues/116 for more details
+ */
+class ConnectionPoolCloseTest {
+
+  private static ListAppender<ILoggingEvent> logCapture = new ListAppender<>();
+
+  private static DataSourcePool pool = createPool(false, false);
+  private static DataSourcePool poolRo = createPool(false, true);
+  private static DataSourcePool poolEnforce = createPool(true, false);
+  private static DataSourcePool poolEnforceRo = createPool(true, true);
+
+
+  @BeforeAll
+  static void before() {
+    // attach logback appender to capture log messages
+    ((ch.qos.logback.classic.Logger) LoggerFactory.getLogger("io.ebean.datasource")).addAppender(logCapture);
+    logCapture.start();
+  }
+
+  @BeforeEach
+  void beforeEach() {
+    logCapture.list.clear();
+  }
+
+  @AfterAll
+  static void after() {
+    logCapture.stop();
+    pool.shutdown();
+    poolRo.shutdown();
+    poolEnforce.shutdown();
+    poolEnforceRo.shutdown();
+  }
+
+  @Test
+  void testNoCommitOrRollback() throws SQLException {
+
+    doNoCommitOrRollback(pool);
+    assertThat(getAndResetLogWarinings())
+      .hasSize(1)
+      .first().asString().startsWith("[WARN] Tried to close a dirty connection at");
+
+    assertThatThrownBy(() -> doNoCommitOrRollback(poolEnforce)).isInstanceOf(AssertionError.class);
+
+    doNoCommitOrRollback(poolRo);
+    assertThat(getAndResetLogWarinings()).isEmpty();
+
+    doNoCommitOrRollback(poolEnforceRo);
+    assertThat(getAndResetLogWarinings()).isEmpty();
+
+  }
+
+  @Test
+  void testCommit() throws SQLException {
+    doCommit(pool);
+    doCommit(poolRo);
+    doCommit(poolEnforce);
+    doCommit(poolEnforceRo);
+    assertThat(getAndResetLogWarinings()).isEmpty();
+  }
+
+  @Test
+  void testRollback() throws SQLException {
+    doRollback(pool);
+    doRollback(poolRo);
+    doRollback(poolEnforce);
+    doRollback(poolEnforceRo);
+    assertThat(getAndResetLogWarinings()).isEmpty();
+  }
+
+  @Test
+  void testExecuteValidSql() throws SQLException {
+    doExecute(pool, "SELECT 1");
+    doExecute(poolRo, "SELECT 1");
+    doExecute(poolEnforce, "SELECT 1");
+    doExecute(poolEnforceRo, "SELECT 1");
+    assertThat(getAndResetLogWarinings()).isEmpty();
+  }
+
+  @Test
+  void testExecuteInvalidSql() throws SQLException {
+    assertThatThrownBy(() -> doExecute(pool, "invalid query"))
+      .isInstanceOf(SQLSyntaxErrorException.class)
+      .hasNoSuppressedExceptions();
+    // (un)fortunately, we will log a missing rollback, when exception in try-with-resourches is thrown.
+    assertThat(getAndResetLogWarinings())
+      .hasSize(1)
+      .first().asString().startsWith("[WARN] Tried to close a dirty connection at");
+
+    assertThatThrownBy(() -> doExecute(poolEnforce, "invalid query"))
+      .isInstanceOf(SQLSyntaxErrorException.class)
+      .hasSuppressedException(new AssertionError("Tried to close a dirty connection. See https://github.com/ebean-orm/ebean-datasource/issues/116 for details."));
+
+    assertThatThrownBy(() -> doExecute(poolRo, "invalid query"))
+      .isInstanceOf(SQLSyntaxErrorException.class)
+      .hasNoSuppressedExceptions();
+
+    assertThatThrownBy(() -> doExecute(poolEnforceRo, "invalid query"))
+      .isInstanceOf(SQLSyntaxErrorException.class)
+      .hasNoSuppressedExceptions();
+    assertThat(getAndResetLogWarinings()).isEmpty();
+  }
+
+  private static void doNoCommitOrRollback(DataSourcePool pool) throws SQLException {
+    try (Connection connection = pool.getConnection()) {
+      // we do nothing here.
+    }
+  }
+
+  private static void doCommit(DataSourcePool pool) throws SQLException {
+    try (Connection connection = pool.getConnection()) {
+      connection.commit();
+    }
+  }
+
+  private static void doRollback(DataSourcePool pool) throws SQLException {
+    try (Connection connection = pool.getConnection()) {
+      connection.rollback();
+    }
+  }
+
+  private static void doExecute(DataSourcePool pool, String query) throws SQLException {
+    try (Connection connection = pool.getConnection()) {
+      connection.createStatement().execute(query);
+      connection.commit();
+    }
+  }
+
+  private static List<ILoggingEvent> getAndResetLogWarinings() {
+    List<ILoggingEvent> warnings = logCapture.list.stream().filter(e -> e.getLevel().levelInt >= Level.WARN_INT).collect(Collectors.toList());
+    logCapture.list.clear();
+    return warnings;
+  }
+
+  private static DataSourcePool createPool(boolean enforceCleanClose, boolean readOnly) {
+    DataSourcePool ret = DataSourceBuilder.create()
+      .url("jdbc:h2:mem:tests")
+      .username("sa")
+      .password("")
+      .minConnections(2)
+      .maxConnections(4)
+      .enforceCleanClose(enforceCleanClose)
+      .readOnly(readOnly)
+      .build();
+    // clear capturer after create
+    logCapture.list.clear();
+    return ret;
+  }
+}