diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/MapRetryContextCache.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/MapRetryContextCache.java index 00b943330..404e4d2cd 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/MapRetryContextCache.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/MapRetryContextCache.java @@ -24,7 +24,8 @@ import org.springframework.batch.retry.RetryContext; /** * Map-based implementation of {@link RetryContextCache}. The map backing the - * cache of contexts is synchronized and its entries are soft-referenced. + * cache of contexts is synchronized and its entries are soft-referenced, so may + * be garbage collected under pressure. * * @author Dave Syer * @@ -37,8 +38,9 @@ public class MapRetryContextCache implements RetryContextCache { * cache with item keys that are inconsistent. */ public static final int DEFAULT_CAPACITY = 4096; - - private Map> map = Collections.synchronizedMap(new HashMap>()); + + private Map> map = Collections + .synchronizedMap(new HashMap>()); private int capacity; @@ -71,6 +73,13 @@ public class MapRetryContextCache implements RetryContextCache { } public boolean containsKey(Object key) { + if (!map.containsKey(key)) { + return false; + } + if (map.get(key).get() == null) { + // our reference was garbage collected + map.remove(key); + } return map.containsKey(key); } @@ -80,7 +89,9 @@ public class MapRetryContextCache implements RetryContextCache { public void put(Object key, RetryContext context) { if (map.size() >= capacity) { - throw new RetryCacheCapacityExceededException("Retry cache capacity limit breached. " + "Do you need to re-consider the implementation of the key generator, " + "or the equals and hashCode of the items that failed?"); + throw new RetryCacheCapacityExceededException("Retry cache capacity limit breached. " + + "Do you need to re-consider the implementation of the key generator, " + + "or the equals and hashCode of the items that failed?"); } map.put(key, new SoftReference(context)); } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java index fccae6c59..2a5e13e4c 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java @@ -236,7 +236,7 @@ public class RetryTemplate implements RetryOperations { doOnErrorInterceptors(retryCallback, context, e); registerThrowable(retryPolicy, state, context, e); - + try { backOffPolicy.backOff(backOffContext); } @@ -247,7 +247,7 @@ public class RetryTemplate implements RetryOperations { logger.debug("Abort retry because interrupted: count=" + context.getRetryCount()); throw ex; } - + if (shouldRethrow(retryPolicy, context, state)) { logger.debug("Rethrow in retry for policy: count=" + context.getRetryCount()); throw e; @@ -350,24 +350,28 @@ public class RetryTemplate implements RetryOperations { if (state.isForceRefresh()) { return doOpenInternal(retryPolicy); } - else if (retryContextCache.containsKey(key)) { - RetryContext context = retryContextCache.get(key); - if (context == null) { + // If there is no cache hit we can avoid the possible expense of the + // cache re-hydration. + if (!retryContextCache.containsKey(key)) { + // The cache is only used if there is a failure. + return doOpenInternal(retryPolicy); + } + + RetryContext context = retryContextCache.get(key); + if (context == null) { + if (retryContextCache.containsKey(key)) { throw new RetryException("Inconsistent state for failed item: no history found. " + "Consider whether equals() or hashCode() for the item might be inconsistent, " + "or if you need to supply a better ItemKeyGenerator"); } - return context; - - } - else { - - // The cache is only used if there is a failure. + // The cache could have been expired in between calls to + // containsKey(), so we have to live with this: return doOpenInternal(retryPolicy); - } + return context; + } /** diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/MapRetryContextCacheTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/MapRetryContextCacheTests.java index 2100d3611..6315977e4 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/MapRetryContextCacheTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/MapRetryContextCacheTests.java @@ -16,20 +16,33 @@ package org.springframework.batch.retry.policy; -import junit.framework.TestCase; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import org.junit.Test; import org.springframework.batch.retry.context.RetryContextSupport; -public class MapRetryContextCacheTests extends TestCase { +public class MapRetryContextCacheTests { MapRetryContextCache cache = new MapRetryContextCache(); - + + @Test public void testPut() { RetryContextSupport context = new RetryContextSupport(null); cache.put("foo", context); assertEquals(context, cache.get("foo")); } + @Test(expected=RetryCacheCapacityExceededException.class) + public void testPutOverLimit() { + RetryContextSupport context = new RetryContextSupport(null); + cache.setCapacity(1); + cache.put("foo", context); + cache.put("foo", context); + } + + @Test public void testRemove() { assertFalse(cache.containsKey("foo")); RetryContextSupport context = new RetryContextSupport(null);