Merge pull request #139 from ebean-orm/feature/137-logical-lock

Fix such that there is a rollback() before LogicalLock unlock()

Author: rbygrave

ebean-migration/src/main/java/io/ebean/migration/runner/MigrationEngine.java

@@ -5,7 +5,6 @@
 import io.ebean.migration.MigrationException;
 import io.ebean.migration.MigrationResource;
 
-import java.io.IOException;
 import java.sql.Connection;
 import java.sql.SQLException;
 import java.util.Collections;
@@ -44,7 +43,6 @@ public List<MigrationResource> run(Connection connection) {
       }
 
       long startMs = System.currentTimeMillis();
-      connection.setAutoCommit(false);
       MigrationTable table = initialiseMigrationTable(connection);
       try {
         List<MigrationResource> result = runMigrations(resources.versions(), table, checkStateOnly);
@@ -58,30 +56,34 @@ public List<MigrationResource> run(Connection connection) {
           }
         }
         return result;
+      } catch (MigrationException e) {
+        rollback(connection);
+        throw e;
+      } catch (Exception e) {
+        log.log(ERROR, "Perform rollback due to DB migration error", e);
+        rollback(connection);
+        throw new MigrationException("Error running DB migrations", e);
       } finally {
         table.unlockMigrationTable();
       }
-
-    } catch (MigrationException e) {
-      rollback(connection);
-      throw e;
-
-    } catch (Exception e) {
-      rollback(connection);
-      throw new MigrationException("Error running DB migrations", e);
-
     } finally {
       close(connection);
     }
   }
 
-  private MigrationTable initialiseMigrationTable(Connection connection) throws SQLException, IOException {
-    MigrationPlatform platform = derivePlatformName(migrationConfig, connection);
-    new MigrationSchema(migrationConfig, connection).createAndSetIfNeeded();
+  private MigrationTable initialiseMigrationTable(Connection connection) {
+    try {
+      connection.setAutoCommit(false);
+      MigrationPlatform platform = derivePlatformName(migrationConfig, connection);
+      new MigrationSchema(migrationConfig, connection).createAndSetIfNeeded();
 
-    final MigrationTable table = new MigrationTable(migrationConfig, connection, checkStateOnly, platform);
-    table.createIfNeededAndLock();
-    return table;
+      final MigrationTable table = new MigrationTable(migrationConfig, connection, checkStateOnly, platform);
+      table.createIfNeededAndLock();
+      return table;
+    } catch (Exception e) {
+      rollback(connection);
+      throw new MigrationException("Error initialising db migrations table", e);
+    }
   }
 
   /**
@@ -144,7 +146,7 @@ private void close(Connection connection) {
   /**
    * Rollback the connection logging if an error occurs.
    */
-  private void rollback(Connection connection) {
+  static void rollback(Connection connection) {
     try {
       if (connection != null) {
         connection.rollback();

ebean-migration/src/main/java/io/ebean/migration/runner/MigrationPlatform.java

@@ -1,6 +1,7 @@
 package io.ebean.migration.runner;
 
 import io.ebean.ddlrunner.DdlDetect;
+import io.ebean.migration.MigrationException;
 
 import java.sql.*;
 import java.util.ArrayList;
@@ -11,6 +12,7 @@
 /**
  * Handle database platform specific locking on db migration table.
  */
+@SuppressWarnings({"SqlDialectInspection", "SqlSourceToSinkFlow"})
 class MigrationPlatform {
 
   private static final System.Logger log = MigrationTable.log;
@@ -30,7 +32,7 @@ DdlDetect ddlDetect() {
     return DdlDetect.NONE;
   }
 
-  void unlockMigrationTable(String sqlTable, Connection connection) throws SQLException {
+  void unlockMigrationTable(String sqlTable, Connection connection) {
     // do nothing by default for select for update case
   }
 
@@ -118,9 +120,14 @@ void lockMigrationTable(String sqlTable, Connection connection) throws SQLExcept
     }
 
     @Override
-    void unlockMigrationTable(String sqlTable, Connection connection) throws SQLException {
-      releaseLogicalLock(sqlTable, connection);
-      connection.commit();
+    void unlockMigrationTable(String sqlTable, Connection connection) {
+      try {
+        releaseLogicalLock(sqlTable, connection);
+        connection.commit();
+      } catch (SQLException e) {
+        MigrationEngine.rollback(connection);
+        throw new MigrationException("Error releasing logical lock for ebean migrations");
+      }
     }
 
     private boolean obtainLogicalLock(String sqlTable, Connection connection) throws SQLException {
@@ -191,10 +198,14 @@ private boolean obtainNamedLock(Connection connection) throws SQLException {
     }
 
     @Override
-    void unlockMigrationTable(String sqlTable, Connection connection) throws SQLException {
-      String hash = Integer.toHexString(connection.getMetaData().getURL().hashCode());
-      try (Statement query = connection.createStatement()) {
-        query.execute("select release_lock('ebean_migration-" + hash + "')");
+    void unlockMigrationTable(String sqlTable, Connection connection) {
+      try {
+        String hash = Integer.toHexString(connection.getMetaData().getURL().hashCode());
+        try (Statement query = connection.createStatement()) {
+          query.execute("select release_lock('ebean_migration-" + hash + "')");
+        }
+      } catch (SQLException e) {
+        throw new MigrationException("Error releasing lock for ebean_migration", e);
       }
     }
   }

ebean-migration/src/main/java/io/ebean/migration/runner/MigrationTable.java

@@ -176,7 +176,7 @@ private void obtainLockWithWait() throws SQLException {
   /**
    * Release a lock on the migration table (MySql, MariaDB only).
    */
-  void unlockMigrationTable() throws SQLException {
+  void unlockMigrationTable() {
     platform.unlockMigrationTable(sqlTable, connection);
   }
 

ebean-migration/src/test/java/io/ebean/migration/MigrationRunnerTest.java

@@ -56,6 +56,37 @@ public void run_when_fileSystemResources() {
     runner.run();
   }
 
+  @Test
+  public void run_when_error() throws SQLException {
+
+    DataSourceConfig dataSourceConfig = new DataSourceConfig();
+    dataSourceConfig.setDriver("org.h2.Driver");
+    dataSourceConfig.setUrl("jdbc:h2:mem:err.db");
+    dataSourceConfig.setUsername("sa");
+    dataSourceConfig.setPassword("");
+
+    DataSourcePool dataSource = DataSourceFactory.create("test", dataSourceConfig);
+
+    MigrationConfig config = createMigrationConfig();
+    config.setMigrationPath("dbmig_error");
+    MigrationRunner runner = new MigrationRunner(config);
+    try {
+      runner.run(dataSource);
+    } catch (Exception expected) {
+      try (Connection connection = dataSource.getConnection()) {
+        try (var pstmt = connection.prepareStatement("select count(*) from m1")) {
+          try (var resultSet = pstmt.executeQuery()) {
+            assertThat(resultSet.next()).isTrue();
+            int val = resultSet.getInt(1);
+            assertThat(val).isEqualTo(0);
+          }
+        } catch (SQLException ex) {
+          fail(ex);
+        }
+      }
+    }
+  }
+
   @Test
   public void run_when_suppliedConnection() {
 

ebean-migration/src/test/resources/dbmig_error/1.1__initial.sql

@@ -0,0 +1 @@
+create table m1 (id integer, acol varchar(20));

ebean-migration/src/test/resources/dbmig_error/1.2__insert.sql

@@ -0,0 +1 @@
+insert into m1 (id, acol) values (1,'hi');

ebean-migration/src/test/resources/dbmig_error/1.3__error.sql

@@ -0,0 +1 @@
+not valid sql