Open GuiSim opened 4 years ago
I see what you mean about the inconsistent state. It looks like catch (Exception e)
should be catch (Throwable t)
at the very least. This same issue exists in the JPA version of guice-persist, and a quick look at their bug tracker shows that some aspect of this has come up before, although I think this issue is slightly different but related to what you're facing: https://github.com/google/guice/issues/692.
Given that @Transactional
is coming from Guice, any change must implement the semantics documented there. The explicit rollbackOn
and ignore
lists only allow for Exception
s. The doc on that annotation says that "By default, only unchecked exceptions trigger a rollback." which should include Error
, but should not include any other Throwable
which isn't an Error
or a RuntimeException
.
Without changing the contract of @Transactional
you'll have to commit-by-default on other "checked" Throwable
s without really providing any way for those to be explicitly ignored via the annotation. This seems like an acceptable compromise given the constraints...anyone who has an issue with this will have to provide exception handling beyond what can be achieved declaratively with the annotation: specifically, if you want to rollback on a checked Throwable
which is not also an Exception
, you'll have to catch and wrap it with an Exception
which can then be included in the rollbackOn
list.
As for the semantics of ignore
, I'll leave you with this gem from the javadoc:
A list of exceptions to not rollback on. A caveat to the rollbackOn clause. The disjunction of rollbackOn and ignore represents the list of exceptions that will trigger a rollback. The complement of rollbackOn and the universal set plus any exceptions in the ignore set represents the list of exceptions that will trigger a commit. Note that ignore exceptions take precedence over rollbackOn, but with subtype granularity.
PR welcome!
Hi @GuiSim , is this issue still causing issues for you? I just noticed it had been awhile since you offered to raise a PR.
Hey!
I've since changed job and don't use this library anymore. Sorry for not providing a PR!
Hello! We chased a nasty bug in our environment recently and one thing that we realized is that JdbcLocalTxnInterceptor doesn't rollback on
Error
.https://github.com/supercargo/guice-persist-jooq/blob/master/src/main/java/com/adamlewis/guice/persist/jooq/JdbcLocalTxnInterceptor.java#L80-L100
This only catches
Exception
. If you have code annotated with@Transactional
that is instrumented by the interceptor and that code throws anError
, you end up in a very strange state:The connection will have
autoCommit
set tofalse
since thecatch
will not be triggered. Thefinally
will be invoked, but theautoCommit
being false prevents it from stopping the unit of work.This means that if you're calling these methods from a Thread Pool (as many REST services do - for example) you might eventually re-use this thread. The thread will already have a unit of work that is still running!
There are many ways this could be solved, but I think it makes sense to rollback on any
Throwable
and not justException
.What do you think? If you agree, I can provide a PR.