diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/SkipListener.java b/spring-batch-core/src/main/java/org/springframework/batch/core/SkipListener.java index c62e1591e..9bf82427a 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/SkipListener.java +++ b/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 diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/ItemSkipPolicyItemHandler.java b/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/ItemSkipPolicyItemHandler.java index b4370e7ee..85bf37d33 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/ItemSkipPolicyItemHandler.java +++ b/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)) { diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ItemSkipPolicyItemHandlerTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ItemSkipPolicyItemHandlerTests.java index cf6a2a917..0f92da835 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ItemSkipPolicyItemHandlerTests.java +++ b/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. */