ghawk1ns · ghawk1ns · May 13, 2017
diff --git a/src/main/java/com/ghawk1ns/CacheLoader.java b/src/main/java/com/ghawk1ns/CacheLoader.java
@@ -6,19 +6,18 @@ public interface CacheLoader<K, V> {
 
     /**
      *
-     * @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<K, V> 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
@@ -76,31 +76,28 @@ private ScheduledFuture<?> init(boolean newCache) {
 
     private void _load() {
         Map<K, V> 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<K, V> 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<K, V> old = cache;
+            try {
+                if (makeImmutable) {
+                    cache = ImmutableMap.copyOf(freshMap);
+                } else {
+                    Map<K, V> 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
@@ -40,7 +40,7 @@ public class CacheMapBuilder<K, V> {
      * @param makeImmutable true if {@link CacheMap} should immutable when not loading
      * @see com.google.common.collect.ImmutableMap
      */
-    public CacheMapBuilder makeImmutable(boolean makeImmutable) {
+    public CacheMapBuilder<K, V> 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<K, V> 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<K, V> 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<K, V> 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<K, V> 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<K, V> loader) {
+    public CacheMapBuilder<K, V> loader(CacheLoader<K, V> loader) {
         this.loader = loader;
         return this;
     }
@@ -110,7 +110,7 @@ public CacheMap<K, V> build() {
     }
 
     // convenience method
-    public static <K, V> CacheMapBuilder newBuilder() {
+    public static <K, V> CacheMapBuilder<K, V> newBuilder() {
         return new CacheMapBuilder<K, V>();
     }
 }
diff --git 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
@@ -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<String, Boolean> cMap = CacheMapBuilder.<String, Boolean>newBuilder()
                 .ttl(10000)
-                .build(new CacheLoader() {
+                .build(new CacheLoader<String, Boolean>() {
                     @Override
-                    public Map load() {
-                        return null;
+                    public Map<String, Boolean> load() {
+                        HashMap<String, Boolean> 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.<String, Long>newBuilder()
                 .ttl(1000)
-                .build(new CacheLoader() {
+                .build(new CacheLoader<String, Long>() {
                     @Override
-                    public Map load() {
+                    public Map<String, Long> load() {
                         HashMap<String, Long> 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.<String, Long>newBuilder()
                 .initialLoadDelay(100) // removes any doubt of a race condition
                 .ttl(ttl)
                 .makeImmutable(true)
-                .build(new CacheLoader() {
+                .build(new CacheLoader<String, Long>() {
                     @Override
-                    public Map load() {
+                    public Map<String, Long> load() {
                         HashMap<String, Long> 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
@@ -3,7 +3,7 @@
 import java.util.Collections;
 import java.util.Map;
 
-public class EmptyCacheLoader implements CacheLoader {
+public class EmptyCacheLoader implements CacheLoader<Object, Object> {
 
     private static final EmptyCacheLoader singleton = new EmptyCacheLoader();