ISIS-1502: prevents infinite loop in IsisTransactionManager#endTransaction() when there is an error in domain logic code
Project: http://git-wip-us.apache.org/repos/asf/isis/repo Commit: http://git-wip-us.apache.org/repos/asf/isis/commit/5692abcb Tree: http://git-wip-us.apache.org/repos/asf/isis/tree/5692abcb Diff: http://git-wip-us.apache.org/repos/asf/isis/diff/5692abcb Branch: refs/heads/master Commit: 5692abcbc0d2c1927cf6ac6cb0c8256e2e342f9f Parents: d532abc Author: Dan Haywood <[email protected]> Authored: Thu Sep 29 10:47:07 2016 +0100 Committer: Dan Haywood <[email protected]> Committed: Thu Sep 29 10:58:01 2016 +0100 ---------------------------------------------------------------------- .../system/transaction/IsisTransaction.java | 2 +- .../transaction/IsisTransactionManager.java | 116 +++++++++++++------ 2 files changed, 80 insertions(+), 38 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/isis/blob/5692abcb/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java ---------------------------------------------------------------------- diff --git a/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java b/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java index f7eaf60..be7a32a 100644 --- a/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java +++ b/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransaction.java @@ -429,7 +429,7 @@ public class IsisTransaction implements TransactionScopedComponent, Transaction } - public void commit() { + void commit() { assert getState().canCommit(); assert abortCause == null; http://git-wip-us.apache.org/repos/asf/isis/blob/5692abcb/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransactionManager.java ---------------------------------------------------------------------- diff --git a/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransactionManager.java b/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransactionManager.java index 4d1f123..560b2a1 100644 --- a/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransactionManager.java +++ b/core/runtime/src/main/java/org/apache/isis/core/runtime/system/transaction/IsisTransactionManager.java @@ -19,6 +19,7 @@ package org.apache.isis.core.runtime.system.transaction; +import java.text.MessageFormat; import java.util.UUID; import org.slf4j.Logger; @@ -118,7 +119,7 @@ public class IsisTransactionManager implements SessionScopedComponent { * {@link IsisTransaction transaction}. * * <p> - * If a transaction is {@link IsisContext#inTransaction() in progress}, then + * If a transaction is in progress, then * uses that. Otherwise will {@link #startTransaction() start} a transaction * before running the block and {@link #endTransaction() commit} it at the * end. @@ -286,22 +287,52 @@ public class IsisTransactionManager implements SessionScopedComponent { * exception in turn. */ public void endTransaction() { - if (LOG.isDebugEnabled()) { - LOG.debug("endTransaction: level " + (transactionLevel) + "->" + (transactionLevel - 1)); - } final IsisTransaction transaction = getCurrentTransaction(); - if (transaction == null || transaction.getState().isComplete()) { + if (transaction == null) { // allow this method to be called >1 with no adverse affects + + if (LOG.isDebugEnabled()) { + LOG.debug("endTransaction: level {} (no transaction exists)", transactionLevel); + } + return; + } else if (transaction.getState().isComplete()) { + // allow this method to be called >1 with no adverse affects + + if (LOG.isDebugEnabled()) { + LOG.debug("endTransaction: level {} (previous transaction completed)", transactionLevel); + } + + return; + } else { + if (LOG.isDebugEnabled()) { + LOG.debug("endTransaction: level {}->{}", transactionLevel, transactionLevel - 1); + } } + + try { + endTransactionInternal(); + } finally { + final IsisTransaction.State state = getCurrentTransaction().getState(); + int transactionLevel = this.transactionLevel; + if(transactionLevel == 0 && !state.isComplete()) { + LOG.error("endTransaction: inconsistency detected between transactionLevel {} and transactionState '{}'", transactionLevel, state); + } + } + } + + private void endTransactionInternal() { + + final IsisTransaction transaction = getCurrentTransaction(); + // terminate the transaction early if an abort cause was already set. RuntimeException abortCause = this.getCurrentTransaction().getAbortCause(); if(transaction.getState().mustAbort()) { - + if (LOG.isDebugEnabled()) { - LOG.debug("endTransaction: aborting instead [EARLY TERMINATION], abort cause '" + abortCause.getMessage() + "' has been set"); + LOG.debug("endTransaction: aborting instead [EARLY TERMINATION], abort cause '{}' has been set", abortCause.getMessage()); } try { abortTransaction(); @@ -309,12 +340,12 @@ public class IsisTransactionManager implements SessionScopedComponent { // just in case any different exception was raised... abortCause = this.getCurrentTransaction().getAbortCause(); } catch(RuntimeException ex) { - + // ... or, capture this most recent exception abortCause = ex; } - - + + if(abortCause != null) { // hasn't been rendered lower down the stack, so fall back throw abortCause; @@ -324,61 +355,66 @@ public class IsisTransactionManager implements SessionScopedComponent { } } - - transactionLevel--; - if (transactionLevel == 0) { + // we don't decrement the transactionLevel just yet, because an exception might end up being thrown + // (meaning there would be more faffing around to ensure that the transactionLevel + // and state of the currentTransaction remain in sync) + if ( (transactionLevel - 1) == 0) { // // TODO: granted, this is some fairly byzantine coding. but I'm trying to account for different types // of object store implementations that could start throwing exceptions at any stage. // once the contract/API for the objectstore is better tied down, hopefully can simplify this... // - + if(abortCause == null) { - + if (LOG.isDebugEnabled()) { LOG.debug("endTransaction: committing"); } try { getCurrentTransaction().preCommit(); - } catch(RuntimeException ex) { + } catch(Exception ex) { // just in case any new exception was raised... - abortCause = ex; - transactionLevel = 1; // because the transactionLevel was decremented earlier + + // this bizarre code because an InvocationTargetException (which is not a RuntimeException) was + // being thrown due to a coding error in a domain object + abortCause = ex instanceof RuntimeException ? (RuntimeException) ex : new RuntimeException(ex); + + getCurrentTransaction().setAbortCause(new IsisTransactionManagerException(ex)); } } - + if(abortCause == null) { try { persistenceSession.endTransaction(); - } catch(RuntimeException ex) { + } catch(Exception ex) { // just in case any new exception was raised... - abortCause = ex; - + abortCause = ex instanceof RuntimeException ? (RuntimeException) ex : new RuntimeException(ex); + // hacky... moving the transaction back to something other than COMMITTED - transactionLevel = 1; // because the transactionLevel was decremented earlier getCurrentTransaction().setAbortCause(new IsisTransactionManagerException(ex)); } } - if(abortCause == null) { - try { - getCurrentTransaction().commit(); - } catch(RuntimeException ex) { - // just in case any new exception was raised... - abortCause = ex; - transactionLevel = 1; // because the transactionLevel was decremented earlier - } - } + + // + // ok, everything that could have thrown an exception is now done, + // so it's safe to decrement the transaction level + // + transactionLevel = 0; // previously we called __isis_endRequest here on all RequestScopedServices. This is now // done later, in PersistenceSession#close(). If we introduce support for @TransactionScoped // services, then this would be the place to finalize them. + + // + // finally, if an exception was thrown, we rollback the transaction + // if(abortCause != null) { - + if (LOG.isDebugEnabled()) { LOG.debug("endTransaction: aborting instead, abort cause has been set"); } @@ -389,17 +425,23 @@ public class IsisTransactionManager implements SessionScopedComponent { // * we want the existing abortCause to be available // * the transactionLevel is correctly now at 0. } - + throw abortCause; + } else { + + // keeping things in sync + getCurrentTransaction().commit(); } + } else if (transactionLevel < 0) { - LOG.error("endTransaction: transactionLevel=" + transactionLevel); + LOG.error("endTransaction: transactionLevel={}", transactionLevel); transactionLevel = 0; - throw new IllegalStateException(" no transaction running to end (transactionLevel < 0)"); + IllegalStateException ex = new IllegalStateException(" no transaction running to end (transactionLevel < 0)"); + getCurrentTransaction().setAbortCause(new IsisException(ex)); + throw ex; } } - public void abortTransaction() { if (getCurrentTransaction() != null) { getCurrentTransaction().markAsAborted();
