Fixes #80 by carefully avoiding the implicit commit when rollback fails

CHANGELOG.md

@@ -6,6 +6,7 @@ Only accretive/fixative changes will be made from now on.
 
 The following changes have been committed to the **master** branch since the 1.0.11 release:
 
+* Fix #80 by avoiding the auto-commit restore after a failed rollback in a failed transaction.
 * Address #78 by documenting the `:connectionInitSql` workaround for HikariCP/PostgreSQL and non-default schemas.
 
 ## Stable Builds

src/next/jdbc/transaction.clj

@@ -27,7 +27,8 @@
         rollback-only  (:rollback-only opts)
         old-autocommit (.getAutoCommit con)
         old-isolation  (.getTransactionIsolation con)
-        old-readonly   (.isReadOnly con)]
+        old-readonly   (.isReadOnly con)
+        restore-ac?    (volatile! true)]
     (io!
      (when isolation
        (.setTransactionIsolation con (isolation isolation-levels)))
@@ -37,12 +38,21 @@
      (try
        (let [result (f con)]
          (if rollback-only
+           (do
+             ;; per #80: if the rollback operation fails, we do not
+             ;; want to try to restore auto-commit...
+             (vreset! restore-ac? false)
              (.rollback con)
+             (vreset! restore-ac? true))
            (.commit con))
          result)
        (catch Throwable t
          (try
+           ;; per #80: if the rollback operation fails, we do not
+           ;; want to try to restore auto-commit...
+           (vreset! restore-ac? false)
            (.rollback con)
+           (vreset! restore-ac? true)
            (catch Throwable rb
              ;; combine both exceptions
              (throw (ex-info (str "Rollback failed handling \""
@@ -56,9 +66,10 @@
          ;; want those to replace any exception currently being
          ;; handled -- and if the connection got closed, we just
          ;; want to ignore exceptions here anyway
-         (try
+         (when @restore-ac?
+           (try ; only restore auto-commit if rollback did not fail
              (.setAutoCommit con old-autocommit)
-           (catch Exception _))
+             (catch Exception _)))
          (when isolation
            (try
              (.setTransactionIsolation con old-isolation)

test/next/jdbc_test.clj

@@ -171,7 +171,17 @@ VALUES ('Pear', 'green', 49, 47)
 INSERT INTO fruit (name, appearance, cost, grade)
 VALUES ('Pear', 'green', 49, 47)
 "]))))
-    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"])))))
+    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"]))))
+    (with-open [con (jdbc/get-connection (ds))]
+      (let [ac (.getAutoCommit con)]
+        (is (= [{:next.jdbc/update-count 1}]
+               (jdbc/with-transaction [t con {:rollback-only true}]
+                 (jdbc/execute! t ["
+INSERT INTO fruit (name, appearance, cost, grade)
+VALUES ('Pear', 'green', 49, 47)
+"]))))
+        (is (= 4 (count (jdbc/execute! con ["select * from fruit"]))))
+        (is (= ac (.getAutoCommit con))))))
   (testing "with-transaction exception"
     (is (thrown? Throwable
            (jdbc/with-transaction [t (ds)]
@@ -180,7 +190,18 @@ INSERT INTO fruit (name, appearance, cost, grade)
 VALUES ('Pear', 'green', 49, 47)
 "])
              (throw (ex-info "abort" {})))))
-    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"])))))
+    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"]))))
+    (with-open [con (jdbc/get-connection (ds))]
+      (let [ac (.getAutoCommit con)]
+        (is (thrown? Throwable
+               (jdbc/with-transaction [t con]
+                 (jdbc/execute! t ["
+INSERT INTO fruit (name, appearance, cost, grade)
+VALUES ('Pear', 'green', 49, 47)
+"])
+                 (throw (ex-info "abort" {})))))
+        (is (= 4 (count (jdbc/execute! con ["select * from fruit"]))))
+        (is (= ac (.getAutoCommit con))))))
   (testing "with-transaction call rollback"
     (is (= [{:next.jdbc/update-count 1}]
            (jdbc/with-transaction [t (ds)]
@@ -190,7 +211,19 @@ VALUES ('Pear', 'green', 49, 47)
 "])]
                (.rollback t)
                result))))
-    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"])))))
+    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"]))))
+    (with-open [con (jdbc/get-connection (ds))]
+      (let [ac (.getAutoCommit con)]
+        (is (= [{:next.jdbc/update-count 1}]
+               (jdbc/with-transaction [t con]
+                 (let [result (jdbc/execute! t ["
+INSERT INTO fruit (name, appearance, cost, grade)
+VALUES ('Pear', 'green', 49, 47)
+"])]
+                   (.rollback t)
+                   result))))
+        (is (= 4 (count (jdbc/execute! con ["select * from fruit"]))))
+        (is (= ac (.getAutoCommit con))))))
   (testing "with-transaction with unnamed save point"
     (is (= [{:next.jdbc/update-count 1}]
            (jdbc/with-transaction [t (ds)]
@@ -201,7 +234,20 @@ VALUES ('Pear', 'green', 49, 47)
 "])]
                (.rollback t save-point)
                result))))
-    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"])))))
+    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"]))))
+    (with-open [con (jdbc/get-connection (ds))]
+      (let [ac (.getAutoCommit con)]
+        (is (= [{:next.jdbc/update-count 1}]
+               (jdbc/with-transaction [t con]
+                 (let [save-point (.setSavepoint t)
+                       result (jdbc/execute! t ["
+INSERT INTO fruit (name, appearance, cost, grade)
+VALUES ('Pear', 'green', 49, 47)
+"])]
+                   (.rollback t save-point)
+                   result))))
+        (is (= 4 (count (jdbc/execute! con ["select * from fruit"]))))
+        (is (= ac (.getAutoCommit con))))))
   (testing "with-transaction with named save point"
     (is (= [{:next.jdbc/update-count 1}]
            (jdbc/with-transaction [t (ds)]
@@ -212,7 +258,20 @@ VALUES ('Pear', 'green', 49, 47)
 "])]
                (.rollback t save-point)
                result))))
-    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"]))))))
+    (is (= 4 (count (jdbc/execute! (ds) ["select * from fruit"]))))
+    (with-open [con (jdbc/get-connection (ds))]
+      (let [ac (.getAutoCommit con)]
+        (is (= [{:next.jdbc/update-count 1}]
+               (jdbc/with-transaction [t con]
+                 (let [save-point (.setSavepoint t (name (gensym)))
+                       result (jdbc/execute! t ["
+INSERT INTO fruit (name, appearance, cost, grade)
+VALUES ('Pear', 'green', 49, 47)
+"])]
+                   (.rollback t save-point)
+                   result))))
+        (is (= 4 (count (jdbc/execute! con ["select * from fruit"]))))
+        (is (= ac (.getAutoCommit con)))))))
 
 (deftest connection-tests
   (testing "datasource via jdbcUrl"