RESOLVED - BATCH-625: SkipListener#onSkipInWrite(..) called multiple times for the same item

onSkipInWrite(..) is now called immediately after it is decided item should be skipped (also updated the SkipListener javadoc).

spring-batch-core/src/main/java/org/springframework/batch/core/SkipListener.java

@@ -18,28 +18,28 @@ package org.springframework.batch.core;
 /**
  * Interface for listener to skipped items. Callbacks will be called by
  * {@link Step} implementations at the appropriate time in the step lifecycle.
- * Callbacks must always be made (where relevant) in a transaction that is still
- * valid: i.e. possibly on a read error, but not on a write error.
  * 
  * @author Dave Syer
+ * @author Robert Kasanicky
  * 
  */
 public interface SkipListener extends StepListener {
 
 	/**
 	 * Callback for a failure on read that is legal, so is not going to be
-	 * re-thrown.
+	 * re-thrown. In case transaction is rolled back and items are re-read, this
+	 * callback will occur repeatedly for the same cause.
 	 * 
-	 * @param t
+	 * @param t cause of the failure
 	 */
 	void onSkipInRead(Throwable t);
 
 	/**
 	 * This item failed on write with the given exception, and a skip was called
-	 * for. The callback is deferred until a new transaction is available. This
-	 * callback might occur more than once for the same item, but only once in
-	 * successful transaction.
-	 * 
+	 * for. The callback occurs immediately after the item is marked for future
+	 * skipping and is called only once for the same item, regardless of
+	 * rollbacks (chunk may be re-processed several times or the exception on
+	 * write may not cause rollback at all).
 	 * 
 	 * @param item the failed item
 	 * @param t the cause of the failure

spring-batch-core/src/main/java/org/springframework/batch/core/step/item/ItemSkipPolicyItemHandler.java

@@ -169,9 +169,7 @@ public class ItemSkipPolicyItemHandler extends SimpleItemHandler {
 				while (item != null && throwable != null) {
 					logger.debug("Skipping item on input, previously failed on output; key=[" + key + "]");
 					scheduleForRemoval(key);
-					if (listener != null) {
-						listener.onSkipInWrite(item, throwable);
-					}
+					
 					item = doRead();
 					key = itemKeyGenerator.getKey(item);
 					throwable = getSkippedException(key);
@@ -239,6 +237,10 @@ public class ItemSkipPolicyItemHandler extends SimpleItemHandler {
 				addSkippedException(key, e);
 				logger.debug("Added item to skip list; key=" + key);
 
+				if (listener != null) {
+					listener.onSkipInWrite(item, e);
+				}
+				
 				// return without re-throwing if exception shouldn't cause
 				// rollback
 				if (!shouldRethrow(e)) {

spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ItemSkipPolicyItemHandlerTests.java

@@ -25,8 +25,10 @@ import junit.framework.TestCase;
 import org.springframework.batch.core.JobExecution;
 import org.springframework.batch.core.JobInstance;
 import org.springframework.batch.core.JobParameters;
+import org.springframework.batch.core.SkipListener;
 import org.springframework.batch.core.StepContribution;
 import org.springframework.batch.core.UnexpectedJobExecutionException;
+import org.springframework.batch.core.listener.SkipListenerSupport;
 import org.springframework.batch.core.step.StepSupport;
 import org.springframework.batch.core.step.skip.AlwaysSkipItemSkipPolicy;
 import org.springframework.batch.item.ClearFailedException;
@@ -233,6 +235,33 @@ public class ItemSkipPolicyItemHandlerTests extends TestCase {
 		assertEquals(new Holder("5"), handler.read(contribution));
 	}
 
+	/**
+	 * {@link SkipListener#onSkipInWrite(Object, Throwable)} should be called
+	 * also if the exception is not re-thrown (does not cause rollback).
+	 */
+	public void testHandleWithSkipAndListeners() throws Exception {
+
+		class SkipOnWriteListener extends SkipListenerSupport {
+
+			boolean called = false;
+
+			public void onSkipInWrite(Object item, Throwable t) {
+				called = true;
+			}
+
+		}
+		;
+		SkipOnWriteListener skipOnWriteListener = new SkipOnWriteListener();
+		handler.setSkipListeners(new SkipListener[] { skipOnWriteListener });
+		handler.setDoNotRethrowExceptionClasses(new Class[] { SkippableException.class });
+		handler.setItemSkipPolicy(new AlwaysSkipItemSkipPolicy());
+		for (int i = 0; i < 5; i++) {
+			handler.handle(contribution);
+		}
+		assertTrue("onSkipInWrite should be called although the exception is not re-thrown", skipOnWriteListener.called);
+
+	}
+
 	/**
 	 * Simple item reader that supports skip functionality.
 	 */