diff --git a/src/main/java/com/ghawk1ns/CacheLoader.java b/src/main/java/com/ghawk1ns/CacheLoader.java index f0523c0..ea9a7d5 100644 --- a/src/main/java/com/ghawk1ns/CacheLoader.java +++ b/src/main/java/com/ghawk1ns/CacheLoader.java @@ -6,19 +6,18 @@ public interface CacheLoader { /** * - * @return a {@link java.util.Map} to be cached - * NullPointerException is returned in {@link CacheLoader#onError(Exception)} if load returns a null + * @return a {@link java.util.Map} to be cached or null if the cache shouldn't be re-loaded */ public Map load(); /** * - * Returns an exception if one occurred while loading the map or if {@link CacheLoader#load()} returns null + * Returns an exception if one occurred while loading the map, it almost certainly won't ever happen. */ public void onError(Exception e); /** - * Called when {@link CacheMap} successfully loads a map returned by {@link CacheLoader#load()} + * Called when {@link CacheMap} successfully loads a map or null returned by {@link CacheLoader#load()} */ public void onLoadComplete(); } diff --git a/src/main/java/com/ghawk1ns/CacheMap.java b/src/main/java/com/ghawk1ns/CacheMap.java index e2cf9f3..8c69ccc 100644 --- a/src/main/java/com/ghawk1ns/CacheMap.java +++ b/src/main/java/com/ghawk1ns/CacheMap.java @@ -76,31 +76,28 @@ private ScheduledFuture init(boolean newCache) { private void _load() { Map freshMap = loader.load(); - if (freshMap == null) { - loader.onError(new NullPointerException("Map returned from CacheMapBuilder.loader() is null")); - return; - } - - boolean success = true; - try { - if (makeImmutable) { - cache = ImmutableMap.copyOf(freshMap); - } else { - Map tmp = new HashMap<>(freshMap.size()); - // copies everything into a new map - freshMap.forEach((k,v) -> tmp.merge(k, v, (unused, freshVal) -> freshVal)); - // atomic transaction so we can pull from the cache while this is happening - cache = tmp; + if (freshMap != null) { + Map old = cache; + try { + if (makeImmutable) { + cache = ImmutableMap.copyOf(freshMap); + } else { + Map tmp = new HashMap<>(freshMap.size()); + // copies everything into a new map + freshMap.forEach((k,v) -> tmp.merge(k, v, (unused, freshVal) -> freshVal)); + // atomic transaction so we can pull from the cache while this is happening + cache = tmp; + } + lastUpdated = System.currentTimeMillis(); + } catch (Exception e) { + // fallback to the previous cache + cache = old; + // pass any exceptions along + loader.onError(e); + return; } - lastUpdated = System.currentTimeMillis(); - } catch (Exception e) { - // pass any exceptions along - loader.onError(e); - success = false; - } - if (success) { - loader.onLoadComplete(); } + loader.onLoadComplete(); } /** diff --git a/src/main/java/com/ghawk1ns/CacheMapBuilder.java b/src/main/java/com/ghawk1ns/CacheMapBuilder.java index a4593cf..f27cca1 100644 --- a/src/main/java/com/ghawk1ns/CacheMapBuilder.java +++ b/src/main/java/com/ghawk1ns/CacheMapBuilder.java @@ -40,7 +40,7 @@ public class CacheMapBuilder { * @param makeImmutable true if {@link CacheMap} should immutable when not loading * @see com.google.common.collect.ImmutableMap */ - public CacheMapBuilder makeImmutable(boolean makeImmutable) { + public CacheMapBuilder makeImmutable(boolean makeImmutable) { this.makeImmutable = makeImmutable; return this; } @@ -49,7 +49,7 @@ public CacheMapBuilder makeImmutable(boolean makeImmutable) { * * @param initialLoadDelay the time in {@link CacheMapBuilder#ttlTimeUnit} units before the first load can occur */ - public CacheMapBuilder initialLoadDelay(long initialLoadDelay) { + public CacheMapBuilder initialLoadDelay(long initialLoadDelay) { this.initialLoadDelay = initialLoadDelay; return this; } @@ -58,7 +58,7 @@ public CacheMapBuilder initialLoadDelay(long initialLoadDelay) { * * @param ttl the time to live lifespan of the cache, {@link CacheLoader#load()} is called at the end of every ttl */ - public CacheMapBuilder ttl(long ttl) { + public CacheMapBuilder ttl(long ttl) { this.ttl = ttl; return this; } @@ -67,7 +67,7 @@ public CacheMapBuilder ttl(long ttl) { * * @param ttlTimeUnit the time unit ttl is observed as. Defaults to {@link TimeUnit#MILLISECONDS} */ - public CacheMapBuilder ttlTimeUnit(TimeUnit ttlTimeUnit) { + public CacheMapBuilder ttlTimeUnit(TimeUnit ttlTimeUnit) { this.ttlTimeUnit = ttlTimeUnit; return this; } @@ -78,7 +78,7 @@ public CacheMapBuilder ttlTimeUnit(TimeUnit ttlTimeUnit) { * otherwise a default is provided: * @see java.util.concurrent.Executors#newSingleThreadScheduledExecutor() */ - public CacheMapBuilder scheduledExecutorService(ScheduledExecutorService scheduledExecutorService) { + public CacheMapBuilder scheduledExecutorService(ScheduledExecutorService scheduledExecutorService) { this.scheduledExecutorService = scheduledExecutorService; return this; } @@ -87,7 +87,7 @@ public CacheMapBuilder scheduledExecutorService(ScheduledExecutorService schedul * Note: Failure to set will result in an {@link IllegalArgumentException} * @see CacheLoader */ - public CacheMapBuilder loader(CacheLoader loader) { + public CacheMapBuilder loader(CacheLoader loader) { this.loader = loader; return this; } @@ -110,7 +110,7 @@ public CacheMap build() { } // convenience method - public static CacheMapBuilder newBuilder() { + public static CacheMapBuilder newBuilder() { return new CacheMapBuilder(); } } \ No newline at end of file diff --git a/src/test/java/com/ghawk1ns/CacheMapBuilderTest.java b/src/test/java/com/ghawk1ns/CacheMapBuilderTest.java index d8bc48d..587a2e8 100644 --- a/src/test/java/com/ghawk1ns/CacheMapBuilderTest.java +++ b/src/test/java/com/ghawk1ns/CacheMapBuilderTest.java @@ -37,8 +37,8 @@ public void noLoaderTest() { CacheMapBuilder.newBuilder() .ttl(1) .build(EmptyCacheLoader.instance()); - } catch (IllegalArgumentException E) { - Assert.fail("Loader was provided but illegal argument was thrown"); + } catch (IllegalArgumentException e) { + Assert.fail(e.getMessage()); } } } diff --git a/src/test/java/com/ghawk1ns/CacheMapTest.java b/src/test/java/com/ghawk1ns/CacheMapTest.java index 9030290..9a28377 100644 --- a/src/test/java/com/ghawk1ns/CacheMapTest.java +++ b/src/test/java/com/ghawk1ns/CacheMapTest.java @@ -5,9 +5,8 @@ import java.util.HashMap; import java.util.Map; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.Semaphore; -import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; public class CacheMapTest { @@ -16,43 +15,56 @@ public class CacheMapTest { @Test public void nullLoadTest() throws InterruptedException { - CountDownLatch latch = new CountDownLatch(1); - CacheMapBuilder.newBuilder() + Semaphore sem = new Semaphore(0); + final AtomicBoolean initial = new AtomicBoolean(true); + CacheMap cMap = CacheMapBuilder.newBuilder() .ttl(10000) - .build(new CacheLoader() { + .build(new CacheLoader() { @Override - public Map load() { - return null; + public Map load() { + HashMap m = null; + // The first time we return a map with a value + if (initial.compareAndSet(true, false)) { + m = new HashMap<>(); + m.put(KEY, true); + } + // The second time load() is called null will be returned + return m; } @Override public void onError(Exception e) { - Assert.assertTrue(e instanceof NullPointerException); - latch.countDown(); + Assert.fail(e.getMessage()); } @Override public void onLoadComplete() { - Assert.fail("We should never complete"); + sem.release(); } }); - try { - latch.await(3, TimeUnit.SECONDS); - } catch (Exception e) { - Assert.fail(e.getMessage()); - } + long beforeUpdate = cMap.getLastUpdated(); + + sem.acquire(); + Assert.assertTrue(cMap.get(KEY)); + long updated = cMap.getLastUpdated(); + Assert.assertTrue(beforeUpdate < updated); + // Make sure KEY is the same after we load a null map + sem.acquire(); + Assert.assertTrue(cMap.get(KEY)); + // Make sure that the old updated time is still the last updated time + Assert.assertEquals(updated, cMap.getLastUpdated()); } @Test public void LoadTest() throws InterruptedException { AtomicLong val = new AtomicLong(); Semaphore sem = new Semaphore(0); - CacheMap cache = CacheMapBuilder.newBuilder() + CacheMap cache = CacheMapBuilder.newBuilder() .ttl(1000) - .build(new CacheLoader() { + .build(new CacheLoader() { @Override - public Map load() { + public Map load() { HashMap m = new HashMap<>(); m.put(KEY, val.getAndIncrement()); return m; @@ -97,13 +109,13 @@ public void stopAndStartTest() { AtomicLong value = new AtomicLong(); Semaphore sem = new Semaphore(0); long ttl = 1000; - CacheMap m = CacheMapBuilder.newBuilder() + CacheMap m = CacheMapBuilder.newBuilder() .initialLoadDelay(100) // removes any doubt of a race condition .ttl(ttl) .makeImmutable(true) - .build(new CacheLoader() { + .build(new CacheLoader() { @Override - public Map load() { + public Map load() { HashMap m = new HashMap<>(); m.put(KEY, value.get()); return m; diff --git a/src/test/java/com/ghawk1ns/EmptyCacheLoader.java b/src/test/java/com/ghawk1ns/EmptyCacheLoader.java index 46d0163..d4708fc 100644 --- a/src/test/java/com/ghawk1ns/EmptyCacheLoader.java +++ b/src/test/java/com/ghawk1ns/EmptyCacheLoader.java @@ -3,7 +3,7 @@ import java.util.Collections; import java.util.Map; -public class EmptyCacheLoader implements CacheLoader { +public class EmptyCacheLoader implements CacheLoader { private static final EmptyCacheLoader singleton = new EmptyCacheLoader();