DaanHoogland commented on code in PR #13925:
URL: https://github.com/apache/cloudstack/pull/13925#discussion_r3828961213
##########
framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java:
##########
@@ -1681,6 +1683,10 @@ public T persist(final T entity) {
throw new CloudRuntimeException("Problem with getting the ec
attribute ", e);
} catch (IllegalAccessException e) {
throw new CloudRuntimeException("Problem with getting the ec
attribute ", e);
+ } finally {
+ if (!committed) {
+ txn.rollback();
Review Comment:
I'd keep the flag + finally over rollback-in-catch, for two reasons:
1. finally covers exception types the catch clauses don't. The try block
does field reflection and SQL value coercion in a loop, so an unchecked
RuntimeException (NPE, ClassCastException, etc.) could skip all three catch
clauses but would still hit finally. Rollback-in-catch only protects
SQLException/NoSuchFieldException/IllegalAccessException; finally protects the
whole try block, which is the actual gap we're closing.
2. TransactionLegacy.rollback() isn't nesting-aware the way commit() is - it
strips every START_TXN marker off the stack and does one physical rollback, so
it needs to run exactly once per persist() call regardless of which branch got
there. A single finally guarantees that; three (and eventually ~20+ once #13926
covers the sibling methods) separate call sites don't, and a future added catch
clause could reintroduce the leak by omission - the same class of mistake that
caused this bug.
Happy to reconsider if there's a case I'm missing, but I think the flag is
earning its keep here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
