Allow transactions on distinct connections in the same thread

[mbezjak]

Is your feature request related to a problem? Please describe.

(jdbc/with-transaction [tx1 data-source-1]
  (jdbc/execute! tx1 ,,,)
  (jdbc/with-transaction [tx2 data-source-2]
    (jdbc/execute! tx2 ,,,)
    (throw (Exception. "An exception happened, so rollback tx2"))))

Should open two separate transactions because those are two separate data sources, with two separate SQL connections. They just happen to execute in the same thread. tx2 should be rolled back due an exception.

Right now, the above snippet is equivalent to

(jdbc/with-transaction [tx1 data-source-1]
  (jdbc/execute! tx1 ,,,)
  (jdbc/execute! data-source-2 ,,,)
  (throw (Exception. "An exception happened, so it'll rollback tx1")))

More context: https://clojurians.slack.com/archives/C1Q164V29/p1720188613556859?thread_ts=1720104756.702669&cid=C1Q164V29

Describe the solution you'd like

next.jdbc.transaction/*active-tx* should probably be a hash-map java.sql.Connection -> boolean, or simply a hash-set of java.sql.Connection objects.

Implementing java.sql.Connection is usually done with proxies, so the key to the map above (element in a set) should probably be the result of unwrap call. Doing the same to javax.sql.DataSource might be needed.

For backwards compability, next.jdbc/active-tx? should return true if the hash-map/hash-set is not empty. 1-arity variant accepting a transactable could be introduced.

Describe alternatives you've considered

I haven't yet started thinking about how to solve the problem above.

[mbezjak]

I got sidetracked a bit by other things. This is still on my mind. As soon as I have the time, I'll come back to this.

[mbezjak]

FYI: This is a variant (not merged yet) that we'll likely use in production. I'll report back after we've run it for a while and if we don't see any problems there.

(ns something
  (:require
   [next.jdbc.protocols :as p]
   [next.jdbc.transaction :as tx])
  (:import
   (java.sql Connection)
   (javax.sql DataSource)))

(defonce ^:private ^:dynamic *active-tx* #{})

(defn transact [con body-fn opts]
  (@#'tx/transact* con body-fn opts))

(extend-protocol p/Transactable
  java.sql.Connection
  (-transact [this body-fn opts]
    (let [raw (if (.isWrapperFor this Connection)
                (.unwrap this Connection)
                this)]
      (if (contains? *active-tx* raw)
        (body-fn this)
        (binding [*active-tx* (conj *active-tx* raw)]
          (transact this body-fn opts)))))

  javax.sql.DataSource
  (-transact [this body-fn opts]
    (let [raw (if (.isWrapperFor this DataSource)
                (.unwrap this DataSource)
                this)]
      (if (contains? *active-tx* raw)
        (with-open [con (p/get-connection this opts)]
          (body-fn con))
        (binding [*active-tx* (conj *active-tx* raw)]
          (with-open [con (p/get-connection this opts)]
            (transact con body-fn opts)))))))

Note that the code above was simplified by assuming the :ignore value of *nested-tx*.

[seancorfield]

@mbezjak Seems strange to track DataSource objects instead of Connection objects in that second case -- each time you call get-connection on a DS, you'll get a distinct, new Connection object.

[mbezjak]

@seancorfield You are right, of course!

With the code above, the following tests would fail:

(try
  (jdbc/with-transaction [tx1 data-source]
    (insert-into-foo tx1)
    (jdbc/with-transaction [tx2 tx1]
      ;; Running this SQL shouldn't commit anything! It's as if `with-transaction` block is not
      ;; there at all.
      (is (= 1 (count-foo tx2))))
    (throw (Exception. "rollback outer!")))
  (catch Exception _))
(is (= 0 (count-foo data-source)))

and

(jdbc/with-transaction [tx1 data-source]
  (is (= 0 (count-foo tx1)))
  (try
    (jdbc/with-transaction [tx2 data-source]
      (insert-into-foo tx2)
      (is (= 1 (count-foo tx2)))
      ;; tx2 is a transaction on its own that can be rollbacked. tx1 shouldn't see anything that
      ;; happened in tx2.
      (throw (Exception. "rollback inner!")))
    (catch Exception _)))
  (is (= 0 (count-foo data-source)))

(I know about {:rollback-only true} opt, but I went with exceptions anyway.)

With that in mind, the updated Transactables:

(ns something
  (:require
   [next.jdbc.protocols :as p]
   [next.jdbc.transaction :as tx])
  (:import
   (java.sql Connection)))

(defonce ^:private ^:dynamic *active-tx* #{})

(defn transact [con body-fn opts]
  (@#'tx/transact* con body-fn opts))

(extend-protocol p/Transactable
  java.sql.Connection
  (-transact [this body-fn opts]
    (let [raw (if (.isWrapperFor this Connection)
                (.unwrap this Connection)
                this)]
      (if (contains? *active-tx* raw)
        (body-fn this)
        (binding [*active-tx* (conj *active-tx* raw)]
          (transact this body-fn opts)))))

  javax.sql.DataSource
  (-transact [this body-fn opts]
    (with-open [con (p/get-connection this opts)]
      (p/-transact con body-fn opts))))

:point_up: is really simple without *nested-tx*.

As before, we haven't merged any of this yet. It's all waiting for me to play around with transactions some more. I'll report back after we've run it for a while and if we don't see any problems.