Reset global rollback-only status when rolling back to savepoint

Issue: SPR-6568

spring-jdbc/src/main/java/org/springframework/jdbc/datasource/JdbcTransactionObjectSupport.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,19 +31,18 @@ import org.springframework.transaction.TransactionUsageException;
 import org.springframework.transaction.support.SmartTransactionObject;
 
 /**
- * Convenient base class for JDBC-aware transaction objects.
- * Can contain a {@link ConnectionHolder}, and implements the
- * {@link org.springframework.transaction.SavepointManager}
- * interface based on that ConnectionHolder.
+ * Convenient base class for JDBC-aware transaction objects. Can contain a
+ * {@link ConnectionHolder} with a JDBC {@code Connection}, and implements the
+ * {@link SavepointManager} interface based on that {@code ConnectionHolder}.
  *
- * <p>Allows for programmatic management of JDBC 3.0
- * {@link java.sql.Savepoint Savepoints}. Spring's
- * {@link org.springframework.transaction.support.DefaultTransactionStatus}
- * will automatically delegate to this, as it autodetects transaction
- * objects that implement the SavepointManager interface.
+ * <p>Allows for programmatic management of JDBC {@link java.sql.Savepoint Savepoints}.
+ * Spring's {@link org.springframework.transaction.support.DefaultTransactionStatus}
+ * automatically delegates to this, as it autodetects transaction objects which
+ * implement the {@link SavepointManager} interface.
  *
  * @author Juergen Hoeller
  * @since 1.1
+ * @see DataSourceTransactionManager
  */
 public abstract class JdbcTransactionObjectSupport implements SavepointManager, SmartTransactionObject {
 
@@ -107,6 +106,10 @@ public abstract class JdbcTransactionObjectSupport implements SavepointManager,
 				throw new NestedTransactionNotSupportedException(
 						"Cannot create a nested transaction because savepoints are not supported by your JDBC driver");
 			}
+			if (conHolder.isRollbackOnly()) {
+				throw new CannotCreateTransactionException(
+						"Cannot create savepoint for transaction which is already marked as rollback-only");
+			}
 			return conHolder.createSavepoint();
 		}
 		catch (SQLException ex) {
@@ -123,6 +126,7 @@ public abstract class JdbcTransactionObjectSupport implements SavepointManager,
 		ConnectionHolder conHolder = getConnectionHolderForSavepoint();
 		try {
 			conHolder.getConnection().rollback((Savepoint) savepoint);
+			conHolder.resetRollbackOnly();
 		}
 		catch (Throwable ex) {
 			throw new TransactionSystemException("Could not roll back to JDBC savepoint", ex);
@@ -151,7 +155,7 @@ public abstract class JdbcTransactionObjectSupport implements SavepointManager,
 		}
 		if (!hasConnectionHolder()) {
 			throw new TransactionUsageException(
-					"Cannot create nested transaction if not exposing a JDBC transaction");
+					"Cannot create nested transaction when not exposing a JDBC transaction");
 		}
 		return getConnectionHolder();
 	}

spring-jdbc/src/test/java/org/springframework/jdbc/datasource/DataSourceTransactionManagerTests.java

@@ -1369,6 +1369,122 @@ public class DataSourceTransactionManagerTests  {
 		verify(con).close();
 	}
 
+	@Test
+	public void testExistingTransactionWithPropagationNestedAndRequiredRollback() throws Exception {
+		DatabaseMetaData md = mock(DatabaseMetaData.class);
+		Savepoint sp = mock(Savepoint.class);
+
+		given(md.supportsSavepoints()).willReturn(true);
+		given(con.getMetaData()).willReturn(md);
+		given(con.setSavepoint("SAVEPOINT_1")).willReturn(sp);
+
+		final TransactionTemplate tt = new TransactionTemplate(tm);
+		tt.setPropagationBehavior(TransactionDefinition.PROPAGATION_NESTED);
+		assertTrue("Hasn't thread connection", !TransactionSynchronizationManager.hasResource(ds));
+		assertTrue("Synchronization not active", !TransactionSynchronizationManager.isSynchronizationActive());
+
+		tt.execute(new TransactionCallbackWithoutResult() {
+			@Override
+			protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
+				assertTrue("Is new transaction", status.isNewTransaction());
+				assertTrue("Isn't nested transaction", !status.hasSavepoint());
+				try {
+					tt.execute(new TransactionCallbackWithoutResult() {
+						@Override
+						protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
+							assertTrue("Has thread connection", TransactionSynchronizationManager.hasResource(ds));
+							assertTrue("Synchronization active", TransactionSynchronizationManager.isSynchronizationActive());
+							assertTrue("Isn't new transaction", !status.isNewTransaction());
+							assertTrue("Is nested transaction", status.hasSavepoint());
+							TransactionTemplate ntt = new TransactionTemplate(tm);
+							ntt.execute(new TransactionCallbackWithoutResult() {
+								@Override
+								protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
+									assertTrue("Has thread connection", TransactionSynchronizationManager.hasResource(ds));
+									assertTrue("Synchronization active", TransactionSynchronizationManager.isSynchronizationActive());
+									assertTrue("Isn't new transaction", !status.isNewTransaction());
+									assertTrue("Is regular transaction", !status.hasSavepoint());
+									throw new IllegalStateException();
+								}
+							});
+						}
+					});
+					fail("Should have thrown IllegalStateException");
+				}
+				catch (IllegalStateException ex) {
+					// expected
+				}
+				assertTrue("Is new transaction", status.isNewTransaction());
+				assertTrue("Isn't nested transaction", !status.hasSavepoint());
+			}
+		});
+
+		assertTrue("Hasn't thread connection", !TransactionSynchronizationManager.hasResource(ds));
+		verify(con).rollback(sp);
+		verify(con).releaseSavepoint(sp);
+		verify(con).commit();
+		verify(con).isReadOnly();
+		verify(con).close();
+	}
+
+	@Test
+	public void testExistingTransactionWithPropagationNestedAndRequiredRollbackOnly() throws Exception {
+		DatabaseMetaData md = mock(DatabaseMetaData.class);
+		Savepoint sp = mock(Savepoint.class);
+
+		given(md.supportsSavepoints()).willReturn(true);
+		given(con.getMetaData()).willReturn(md);
+		given(con.setSavepoint("SAVEPOINT_1")).willReturn(sp);
+
+		final TransactionTemplate tt = new TransactionTemplate(tm);
+		tt.setPropagationBehavior(TransactionDefinition.PROPAGATION_NESTED);
+		assertTrue("Hasn't thread connection", !TransactionSynchronizationManager.hasResource(ds));
+		assertTrue("Synchronization not active", !TransactionSynchronizationManager.isSynchronizationActive());
+
+		tt.execute(new TransactionCallbackWithoutResult() {
+			@Override
+			protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
+				assertTrue("Is new transaction", status.isNewTransaction());
+				assertTrue("Isn't nested transaction", !status.hasSavepoint());
+				try {
+					tt.execute(new TransactionCallbackWithoutResult() {
+						@Override
+						protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
+							assertTrue("Has thread connection", TransactionSynchronizationManager.hasResource(ds));
+							assertTrue("Synchronization active", TransactionSynchronizationManager.isSynchronizationActive());
+							assertTrue("Isn't new transaction", !status.isNewTransaction());
+							assertTrue("Is nested transaction", status.hasSavepoint());
+							TransactionTemplate ntt = new TransactionTemplate(tm);
+							ntt.execute(new TransactionCallbackWithoutResult() {
+								@Override
+								protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
+									assertTrue("Has thread connection", TransactionSynchronizationManager.hasResource(ds));
+									assertTrue("Synchronization active", TransactionSynchronizationManager.isSynchronizationActive());
+									assertTrue("Isn't new transaction", !status.isNewTransaction());
+									assertTrue("Is regular transaction", !status.hasSavepoint());
+									status.setRollbackOnly();
+								}
+							});
+						}
+					});
+					fail("Should have thrown UnexpectedRollbackException");
+				}
+				catch (UnexpectedRollbackException ex) {
+					// expected
+				}
+				assertTrue("Is new transaction", status.isNewTransaction());
+				assertTrue("Isn't nested transaction", !status.hasSavepoint());
+			}
+		});
+
+		assertTrue("Hasn't thread connection", !TransactionSynchronizationManager.hasResource(ds));
+		verify(con).rollback(sp);
+		verify(con).releaseSavepoint(sp);
+		verify(con).commit();
+		verify(con).isReadOnly();
+		verify(con).close();
+	}
+
 	@Test
 	public void testExistingTransactionWithManualSavepoint() throws Exception {
 		DatabaseMetaData md = mock(DatabaseMetaData.class);

spring-orm/src/main/java/org/springframework/orm/jpa/JpaTransactionManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -677,12 +677,17 @@ public class JpaTransactionManager extends AbstractPlatformTransactionManager
 
 		@Override
 		public Object createSavepoint() throws TransactionException {
+			if (this.entityManagerHolder.isRollbackOnly()) {
+				throw new CannotCreateTransactionException(
+						"Cannot create savepoint for transaction which is already marked as rollback-only");
+			}
 			return getSavepointManager().createSavepoint();
 		}
 
 		@Override
 		public void rollbackToSavepoint(Object savepoint) throws TransactionException {
 			getSavepointManager().rollbackToSavepoint(savepoint);
+			this.entityManagerHolder.resetRollbackOnly();
 		}
 
 		@Override

spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java

@@ -693,20 +693,15 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran
 			if (defStatus.isDebug()) {
 				logger.debug("Transactional code has requested rollback");
 			}
-			processRollback(defStatus);
+			processRollback(defStatus, false);
 			return;
 		}
+
 		if (!shouldCommitOnGlobalRollbackOnly() && defStatus.isGlobalRollbackOnly()) {
 			if (defStatus.isDebug()) {
 				logger.debug("Global transaction is marked as rollback-only but transactional code requested commit");
 			}
-			processRollback(defStatus);
-			// Throw UnexpectedRollbackException only at outermost transaction boundary
-			// or if explicitly asked to.
-			if (status.isNewTransaction() || isFailEarlyOnGlobalRollbackOnly()) {
-				throw new UnexpectedRollbackException(
-						"Transaction rolled back because it has been marked as rollback-only");
-			}
+			processRollback(defStatus, true);
 			return;
 		}
 
@@ -722,30 +717,35 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran
 	private void processCommit(DefaultTransactionStatus status) throws TransactionException {
 		try {
 			boolean beforeCompletionInvoked = false;
+
 			try {
+				boolean unexpectedRollback = false;
 				prepareForCommit(status);
 				triggerBeforeCommit(status);
 				triggerBeforeCompletion(status);
 				beforeCompletionInvoked = true;
-				boolean globalRollbackOnly = false;
-				if (status.isNewTransaction() || isFailEarlyOnGlobalRollbackOnly()) {
-					globalRollbackOnly = status.isGlobalRollbackOnly();
-				}
+
 				if (status.hasSavepoint()) {
 					if (status.isDebug()) {
 						logger.debug("Releasing transaction savepoint");
 					}
+					unexpectedRollback = status.isGlobalRollbackOnly();
 					status.releaseHeldSavepoint();
 				}
 				else if (status.isNewTransaction()) {
 					if (status.isDebug()) {
 						logger.debug("Initiating transaction commit");
 					}
+					unexpectedRollback = status.isGlobalRollbackOnly();
 					doCommit(status);
 				}
+				else if (isFailEarlyOnGlobalRollbackOnly()) {
+					unexpectedRollback = status.isGlobalRollbackOnly();
+				}
+
 				// Throw UnexpectedRollbackException if we have a global rollback-only
 				// marker but still didn't get a corresponding exception from commit.
-				if (globalRollbackOnly) {
+				if (unexpectedRollback) {
 					throw new UnexpectedRollbackException(
 							"Transaction silently rolled back because it has been marked as rollback-only");
 				}
@@ -803,7 +803,7 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran
 		}
 
 		DefaultTransactionStatus defStatus = (DefaultTransactionStatus) status;
-		processRollback(defStatus);
+		processRollback(defStatus, false);
 	}
 
 	/**
@@ -812,10 +812,13 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran
 	 * @param status object representing the transaction
 	 * @throws TransactionException in case of rollback failure
 	 */
-	private void processRollback(DefaultTransactionStatus status) {
+	private void processRollback(DefaultTransactionStatus status, boolean unexpected) {
 		try {
+			boolean unexpectedRollback = unexpected;
+
 			try {
 				triggerBeforeCompletion(status);
+
 				if (status.hasSavepoint()) {
 					if (status.isDebug()) {
 						logger.debug("Rolling back transaction to savepoint");
@@ -828,28 +831,42 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran
 					}
 					doRollback(status);
 				}
-				else if (status.hasTransaction()) {
-					if (status.isLocalRollbackOnly() || isGlobalRollbackOnParticipationFailure()) {
-						if (status.isDebug()) {
-							logger.debug("Participating transaction failed - marking existing transaction as rollback-only");
+				else {
+					// Participating in larger transaction
+					if (status.hasTransaction()) {
+						if (status.isLocalRollbackOnly() || isGlobalRollbackOnParticipationFailure()) {
+							if (status.isDebug()) {
+								logger.debug("Participating transaction failed - marking existing transaction as rollback-only");
+							}
+							doSetRollbackOnly(status);
+						}
+						else {
+							if (status.isDebug()) {
+								logger.debug("Participating transaction failed - letting transaction originator decide on rollback");
+							}
 						}
-						doSetRollbackOnly(status);
 					}
 					else {
-						if (status.isDebug()) {
-							logger.debug("Participating transaction failed - letting transaction originator decide on rollback");
-						}
+						logger.debug("Should roll back transaction but cannot - no transaction available");
+					}
+					// Unexpected rollback only matters here if we're asked to fail early
+					if (!isFailEarlyOnGlobalRollbackOnly()) {
+						unexpectedRollback = false;
 					}
-				}
-				else {
-					logger.debug("Should roll back transaction but cannot - no transaction available");
 				}
 			}
 			catch (RuntimeException | Error ex) {
 				triggerAfterCompletion(status, TransactionSynchronization.STATUS_UNKNOWN);
 				throw ex;
 			}
+
 			triggerAfterCompletion(status, TransactionSynchronization.STATUS_ROLLED_BACK);
+
+			// Raise UnexpectedRollbackException if we had a global rollback-only marker
+			if (unexpectedRollback) {
+				throw new UnexpectedRollbackException(
+						"Transaction rolled back because it has been marked as rollback-only");
+			}
 		}
 		finally {
 			cleanupAfterCompletion(status);

spring-tx/src/main/java/org/springframework/transaction/support/ResourceHolderSupport.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,9 +23,9 @@ import org.springframework.transaction.TransactionTimedOutException;
 /**
  * Convenient base class for resource holders.
  *
- * <p>Features rollback-only support for nested transactions.
- * Can expire after a certain number of seconds or milliseconds,
- * to determine transactional timeouts.
+ * <p>Features rollback-only support for participating transactions.
+ * Can expire after a certain number of seconds or milliseconds
+ * in order to determine a transactional timeout.
  *
  * @author Juergen Hoeller
  * @since 02.02.2004
@@ -66,6 +66,17 @@ public abstract class ResourceHolderSupport implements ResourceHolder {
 		this.rollbackOnly = true;
 	}
 
+	/**
+	 * Reset the rollback-only status for this resource transaction.
+	 * <p>Only really intended to be called after custom rollback steps which
+	 * keep the original resource in action, e.g. in case of a savepoint.
+	 * @since 5.0
+	 * @see org.springframework.transaction.SavepointManager#rollbackToSavepoint
+	 */
+	public void resetRollbackOnly() {
+		this.rollbackOnly = false;
+	}
+
 	/**
 	 * Return whether the resource transaction is marked as rollback-only.
 	 */