Handle nil data correctly

Our policy is: OriginLoader and PeerLoader must always return nil rather than
[]byte{} to indicate zero-length data. This is recommended in the wiki:
https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices.
Get() will similarly return nil when the length is 0.

Closes #7.

cache.go

@@ -90,26 +90,27 @@ func (b *Base) Configure(opts *ConfigureOpts) *Cache {
 }
 
 // Get retrieves an element from the cache, returning the data along with its
-// TTL. Our guarantee is to check the TTL before handing the value back, and
-// reload the key if the TTL has been reached. Due to thread scheduling, we
-// cannot promise to never return a TTL that has expired.  When retrieving a
-// fresh value, we don't check the expiry, as then values may never get back to
-// the user. Internal processing time is not deducted from the TTL. It is not
-// possible for the caller to know whether the value was fetched or already
-// cached; that is only exposed in metrics in aggregate.
+// TTL. The TTL is checked before handing the value back, and the key reloaded
+// if it has expired. Due to thread scheduling, we cannot promise to never
+// return a TTL that has expired. When retrieving a fresh value, the expiry is
+// not checked again to avoid going into a loop. Internal processing time is
+// not deducted from the TTL. It is not possible for the caller to know whether
+// the value was fetched or already cached; that is only exposed in metrics in
+// aggregate. Zero-length data is returned as nil; a non-nil value will have a
+// length of at least 1.
 func (c *Cache) Get(ctx context.Context, key string) ([]byte, lifetime.Lifetime, error) {
 	timer := prometheus.NewTimer(c.Base.getDuration)
 	defer timer.ObserveDuration()
 
 	// check authoritative and hot outside singleflight - this is more
 	// lightweight
 	c.Base.authoritativeGets.Inc()
-	if d, lt := c.tryLRU(key, c.Base.authoritative); d != nil {
+	if d, lt, ok := c.tryLRU(key, c.Base.authoritative); ok {
 		return d, lt, nil
 	}
 	c.Base.authoritativeMisses.Inc()
 	c.Base.hotGets.Inc()
-	if d, lt := c.tryLRU(key, c.Base.hot); d != nil {
+	if d, lt, ok := c.tryLRU(key, c.Base.hot); ok {
 		return d, lt, nil
 	}
 	c.Base.hotMisses.Inc()
@@ -126,14 +127,14 @@ func (c *Cache) Get(ctx context.Context, key string) ([]byte, lifetime.Lifetime,
 		// we must re-check the authoritative and hot caches in case another
 		// goroutine has already loaded the key
 		c.Base.authoritativeGets.Inc()
-		if d, lt := c.tryLRU(key, c.Base.authoritative); d != nil {
+		if d, lt, ok := c.tryLRU(key, c.Base.authoritative); ok {
 			return d, lt, nil
 		}
 		// it may have been a hit, but a TTL override caused it to effectively
 		// be a miss
 		c.Base.authoritativeMisses.Inc()
 		c.Base.hotGets.Inc()
-		if d, lt := c.tryLRU(key, c.Base.hot); d != nil {
+		if d, lt, ok := c.tryLRU(key, c.Base.hot); ok {
 			return d, lt, nil
 		}
 		c.Base.hotMisses.Inc()
@@ -194,30 +195,32 @@ func (c *Cache) capLifetime(key string, lt lifetime.Lifetime) lifetime.Lifetime
 	return lt
 }
 
-func (c *Cache) tryLRU(key string, lruc *lru.Cache) ([]byte, lifetime.Lifetime) {
+// tryLRU attempts to find a key in the provided LRU cache, accounting for
+// lifetime overrides.
+func (c *Cache) tryLRU(key string, lruc *lru.Cache) ([]byte, lifetime.Lifetime, bool) {
 	d, lt, ok := lruc.Get(key)
 	if !ok {
-		return nil, lifetime.Zero
+		return nil, lifetime.Zero, false
 	}
 
-	// if lt.Expired() here, an override could only make it "more" expired. We
-	// don't remove it from the cache as a remove followed by an update (we
+	// if the value has expired, an override could only make it "more" expired.
+	// We don't remove it from the cache as a remove followed by an update (we
 	// assume we will successfully retrieve the new value) is more expensive
 	// than an update in place.
 
 	// adjust for override if one is configured for the key
 	lt = c.capLifetime(key, lt)
 	if lt.Expired() {
 		// possibly due only to the override in place
-		return nil, lifetime.Zero
+		return nil, lifetime.Zero, false
 	}
 
 	// overridden TTLs are not updated in the LRU caches - we overlay them after
 	// items are retrieved. When the override is removed, the original TTL
 	// becomes visible again. As an override can only shorten a TTL, this means
 	// we don't prematurely expire what would otherwise be valid values - we
 	// assume overrides are shortlived.
-	return d, lt
+	return d, lt, true
 }
 
 func (c *Cache) peerLoad(ctx context.Context, node *memberlist.Node, key string) ([]byte, lifetime.Lifetime, error) {

internal/pkg/lru/lru.go

@@ -12,6 +12,7 @@ import (
 // successful result of an OriginLoader.Load() call. It is a value type within
 // the cache.
 type value struct {
+	// data is allowed to be nil.
 	data     []byte
 	lifetime lifetime.Lifetime
 }
@@ -84,9 +85,9 @@ func NewCache(capacity uint, onEvict EvictionFunc) *Cache {
 	}
 }
 
-// Get retrieves the values stored under a key. If the key does not exist, the
-// bool will be false, in which case other return values should be ignored. The
-// returned values must not be modified.
+// Get retrieves the value stored under a key. If the key does not exist, the
+// bool will be false, in which case other return values should be ignored. A
+// key may contain nil data. The returned values must not be modified.
 func (c *Cache) Get(key string) ([]byte, lifetime.Lifetime, bool) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
@@ -99,7 +100,7 @@ func (c *Cache) Get(key string) ([]byte, lifetime.Lifetime, bool) {
 }
 
 // Put adds, or updates the value stored under, a key. It returns whether the
-// key already existed in the cache.
+// key already existed in the cache. Data may be nil.
 func (c *Cache) Put(key string, data []byte, lt lifetime.Lifetime) bool {
 	if c.capacity == 0 {
 		return false

originloader.go

@@ -16,16 +16,10 @@ type OriginLoader interface {
 	// SourceLoader was considered for this, however source is relative; the
 	// point is the source is the *original* source - not another peer.
 
-	// Load produces the value for a key, along with its lifetime. This is
-	// similar to Sink in groupcache. Values are cached amongst peers, and not
-	// reloaded until their TTL has been reached. The TTL returned can be
-	// retrospectively modified via an override, which allows decreasing the
-	// value, however this is only intended for emergencies in very limited
-	// scenarios. Infinite TTLs can be emulated by returning
-	// lifetime.New(lifetime.MaxDuration), however if you do not want TTLs, you
-	// may want to consider another library. This method may be called on any
-	// instance in the cluster for any key at any time, so should return a
-	// deterministic, consistent value.
+	// Load produces the value for a key, along with its lifetime. Zero-length
+	// data must be returned as nil. This method may be called for any key on
+	// any instance in the cluster at any time, so should return a
+	// deterministic, consistent value. This is similar to Sink in groupcache.
 	//
 	// Note the expiry of the context is defined when configuring a base cache,
 	// not by incoming external requests. We deduplicate loads, so want to avoid
@@ -38,6 +32,13 @@ type OriginLoader interface {
 	// that can be tolerated. Remember the jitter must be deterministic, so use
 	// a value derived from the key, e.g. crc32, rather than math/rand.
 	//
+	// Values are cached amongst peers, and not reloaded until their TTL has
+	// been reached. The TTL returned can be retrospectively modified via an
+	// override, which allows decreasing the value, however this is only
+	// intended for emergencies in very limited scenarios. Infinite TTLs can be
+	// emulated by returning lifetime.New(lifetime.MaxDuration), however if you
+	// do not want TTLs, you may want to consider another library.
+	//
 	// Excluding failure scenarios, this will only be called once for a given
 	// key, by the cluster member that owns that key. If that cluster member
 	// dies, its keys will be lazily re-retrieved as necessary. Concurrent

peerloader.go

@@ -19,7 +19,8 @@ type PeerLoader interface {
 
 	// Load requests a key from the specified peer, which will currently always
 	// be the authoritative peer. This peer should call cache.Get(), which may
-	// result in a load via the OriginLoader.
+	// result in a load via the OriginLoader. Zero-length data must be returned
+	// as nil.
 	Load(ctx context.Context, from *memberlist.Node, cache *Cache, key string) ([]byte, lifetime.Lifetime, error)
 }
 

pkg/rpc/loader.go

@@ -108,6 +108,10 @@ func Loader(cache string, client *http.Client, basePath string, perAttempt time.
 		if err != nil {
 			return nil, lifetime.Zero, err
 		}
+		if len(value) == 0 {
+			// we are required to return nil rather than a zero-length []byte
+			return nil, lt, nil
+		}
 		return value, lt, nil
 	})
 }