[java.jdbc] with-db-transaction commits partial transaction

[coocood]

Describe the bug The java.jdbc repo doesn't have the issue section, so I'm sorry to report the issue here. And maybe next-jdbc has this issue too.

MySQL implicitly commits a transaction if set autocommit = 1 and it's not already 1. Executes this statement in a finally block may break the transaction atomicity.

https://github.com/clojure/java.jdbc/blob/master/src/main/clojure/clojure/java/jdbc.clj#L829

To Reproduce This issue is not easily reproducible, it was found by a jepsen test. https://github.com/jepsen-io/jepsen/issues/428

Expected behavior A clear and concise description of what you expected to happen.

Stacktraces If applicable, add stack traces to help explain your problem.

project.clj/deps.edn If possible, please include your Leiningen or clj dependencies, at least for Clojure, next.jdbc, and the database drivers you are using.

Environment (please complete the following information):

• OS: Linux
• Java Version: [e.g. (AdoptOpenJDK)(build 1.8.0_192-b12)]
• Clojure Version: [e.g. 1.10.1]
• Database: [e.g., TiDB master]
• Driver Library Version: [e.g., mysql/mysql-connector-java 5.1.41]

[seancorfield]

I've commented on the Jepsen issue with an explanation of what I think might be happening but also asking there for feedback on how to address the issue since it seems to be quite an edge case and I don't know what would be more correct or more problematic in terms of trying to fix it.

Yes, you're right that next.jdbc could well suffer from the same bug: https://github.com/seancorfield/next-jdbc/blob/master/src/next/jdbc/transaction.clj#L60 but it also does not try to wrap every action in a transaction -- it just relies on the auto-commit status of the active connection and it also makes no attempt to simulate "nesting" (merging) of transactions. I think that would make it less susceptible to this edge case bug?

FYI: clojure.java.jdbc tracks issues on JIRA because it is a Contrib library: https://clojure.atlassian.net/projects/JDBC/issues -- if you don't have a JIRA account, you can report bugs on ask.clojure.org (see the CONTRIBUTING doc in the repo, which links to https://clojure.org/community/contributing for the full process).

Given that java.jdbc is no longer under "active" maintenance -- it is considered "stable" -- I'm very unlikely to attempt to fix this issue since it is a rare edge case. If it can be reproduced on next.jdbc -- and the discussion on that Jepsen issue gives a clear path toward a solution -- then I would address it in next.jdbc.

[seancorfield]

I'll also note that's a pretty old MySQL driver -- next.jdbc is currently tested against 8.0.18 (which is also what I'm using very heavily at work).

[seancorfield]

Proposed solution: move the restore of auto-commit into the two "happy paths" -- after the work is done in the main try and also after the rollback in the recovery catch/try.

[seancorfield]

It turned out to be a bit more complicated than that -- you only want to avoid the attempted restore if a rollback fails, not if the commit fails I think (if the commit fails, the restore of auto-commit will essentially attempt another commit which will either also fail or will succeed, in which case the overall commit should be completed, I would think?). Either way, it certainly will avoid the problem of the operation failing, and the attempted rollback failing, and then an incorrect commit happening!