I think the correct behavior would be to call rollback on the transaction and not just not calling commit? The user will expect at the end of the transaction block one of commit or rollback to be called.

I was thinking about how to test this. How do you feel about a mock Connection object, where we can check if commit and/or rollback have been called as part of the test?

The same mechanism could be used to test other features.

I was thinking in something like the attached for testing the issue (and other functionality).
It needs several improvements, but it's enough to get the idea.
What do you think?

FYI, here's the IRC discussion about this, for the record:

[5:11pm] <paraseba>
In clojure.java.jdbc there is a transaction function, which is supposed to rollback if the passed body throws an exception, but it only does that in case of an Exception, not any Throwable. What's the rationale behind this?
[5:11pm] <paraseba>
I'd want my transaction rolledback in case of any errors
[5:12pm] <paraseba>
seancorfield: maybe you can explain me this?
[5:53pm] <amalloy>
paraseba: catching Errors is generally a bad practice. i'm not saying it's wrong all the time, but Errors are often unrecoverable anyway
[5:53pm] <amalloy>
eg, "You ran out of heap space! I can't even allocate memory to throw an exception!"
[5:54pm] <paraseba>
but, even in the worst conditions, shouldn't we try to rollback the transaction? is not better that commiting in this unexpected error condition?
[5:55pm] <paraseba>
we can then rethrow the Throwable, after trying to rollback
[5:55pm] <lucian>
paraseba: i don't think the db will commit if it gives you an error
[5:55pm] <lucian>
and as amalloy, errors like OOM are best solved with exit()
[5:55pm] <paraseba>
lucian: it will ... jdbc is catching Exception and rolling back in that case .... but it commits in a finally
[5:56pm] <paraseba>
so, if you have an Error thrown, it will commit
[5:56pm] <paraseba>
I guess that's more surprising than a rollback
[5:57pm] <paraseba>
the logic is .... (try do-your-thing (catch Exception roll-back) (finally commit))
[5:57pm] <lucian>
paraseba: well, then maybe you don't want to commit in finally?
[5:57pm] <paraseba>
I don't, not if I got an Error
[5:58pm] <amalloy>
lucian: i think he's upset that a library he's using is committing on error
[5:59pm] <paraseba>
amalloy: I can solve it easily by wrapping my operations in a catch Throwable -> rollback -> rethrow, but I think it's not the right behavior for the library
[5:59pm] <paraseba>
I would expect a commit only if the block ended without any kind of exceptions or errors. don't you agree ?
[6:01pm] <amalloy>
paraseba: meh. all kinds of weird stuff can happen if an Error occurs; i wouldn't be entirely certain that an attempt to "recover" makes things worse due to some weird program state caused by the Error. i mean, my completely-unresearched opinion is that catching Throwable would be better, but you can't really rely for your program's correctness on anything that happens after an Error
[6:02pm] <paraseba>
but, we are forcing a commit after an Error
[6:04pm] <paraseba>
the usual logic should be .... (do do-your-thing (commit)) if do-your-thing throws anything ... no commit is done. Puting a commit in a finally enforces the commit, even after Errors
[6:06pm] <amalloy>
yeah, i think i agree
[6:08pm] <paraseba>
amalloy: ok, I'll report an issue, thanks

It's important to realize that there are 2 problems here.

a.- Transaction not rolled back on Throwable
b.- The original Exception is "swallowed" and a new, different one is thrown

I think both are equally serious.

I agree Sebastian. I've raised this issue for input from clojure-dev (looking for stylistic guidelines for contrib libraries) because I think both problems need to be fixed.

My current leaning is toward supporting three paths:

• success: rollback transaction if user code sets rollback true, else commit
• Exception: rollback transaction and rethrow (without wrapping)
• Throwable: allow it to escape, take no action

I'm attaching a patch that fixes the problem without adding any tests.

On success, commit unless rollback has been set (in which case rollback).
On Exception, rollback and rethrow as-is.
On Error (Throwable), allow it to escape.
In all cases, set rollback false and restore auto-commit setting.

The only two changes here should be:

• Exception is no longer wrapped when thrown (may break code that expected the wrapping?)
• No commit attempted on Error (Throwable)