Improve HTTP error handling (#60)

CHANGELOG

@@ -4,7 +4,9 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-# 
+# This change log is deprecated in favor of github release functionality.
+# See https://github.com/mailgun/groupcache/releases for recent change activity.
+
 ## [2.3.1] - 2022-05-17
 ### Changed
 * Fix example in README #40

errors.go

@@ -0,0 +1,33 @@
+package groupcache
+
+// ErrNotFound should be returned from an implementation of `GetterFunc` to indicate the
+// requested value is not available. When remote HTTP calls are made to retrieve values from
+// other groupcache instances, returning this error will indicate to groupcache that the
+// value requested is not available, and it should NOT attempt to call `GetterFunc` locally.
+type ErrNotFound struct {
+	Msg string
+}
+
+func (e *ErrNotFound) Error() string {
+	return e.Msg
+}
+
+func (e *ErrNotFound) Is(target error) bool {
+	_, ok := target.(*ErrNotFound)
+	return ok
+}
+
+// ErrRemoteCall is returned from `group.Get()` when an error that is not a `ErrNotFound`
+// is returned during a remote HTTP instance call
+type ErrRemoteCall struct {
+	Msg string
+}
+
+func (e *ErrRemoteCall) Error() string {
+	return e.Msg
+}
+
+func (e *ErrRemoteCall) Is(target error) bool {
+	_, ok := target.(*ErrRemoteCall)
+	return ok
+}

groupcache.go

@@ -392,8 +392,17 @@ func (g *Group) load(ctx context.Context, key string, dest Sink) (value ByteView
 			if err == nil {
 				g.Stats.PeerLoads.Add(1)
 				return value, nil
-			} else if errors.Is(err, context.Canceled) {
-				// do not count context cancellation as a peer error
+			}
+
+			if errors.Is(err, context.Canceled) {
+				return nil, err
+			}
+
+			if errors.Is(err, &ErrNotFound{}) {
+				return nil, err
+			}
+
+			if errors.Is(err, &ErrRemoteCall{}) {
 				return nil, err
 			}
 
@@ -412,10 +421,6 @@ func (g *Group) load(ctx context.Context, key string, dest Sink) (value ByteView
 				// since the context is no longer valid
 				return nil, err
 			}
-			// TODO(bradfitz): log the peer's error? keep
-			// log of the past few for /groupcachez?  It's
-			// probably boring (normal task movement), so not
-			// worth logging I imagine.
 		}
 
 		value, err = g.getLocally(ctx, key, dest)

http.go

@@ -19,6 +19,7 @@ package groupcache
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -225,7 +226,11 @@ func (p *HTTPPool) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	value := AllocatingByteSliceSink(&b)
 	err := group.Get(ctx, key, value)
 	if err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		if errors.Is(err, &ErrNotFound{}) {
+			http.Error(w, err.Error(), http.StatusNotFound)
+			return
+		}
+		http.Error(w, err.Error(), http.StatusServiceUnavailable)
 		return
 	}
 
@@ -299,7 +304,17 @@ func (h *httpGetter) Get(ctx context.Context, in *pb.GetRequest, out *pb.GetResp
 	}
 	defer res.Body.Close()
 	if res.StatusCode != http.StatusOK {
-		msg, _ := ioutil.ReadAll(io.LimitReader(res.Body, 1024*1024)) // Limit reading the error body to max 1 MiB
+		// Limit reading the error body to max 1 MiB
+		msg, _ := io.ReadAll(io.LimitReader(res.Body, 1024*1024))
+
+		if res.StatusCode == http.StatusNotFound {
+			return &ErrNotFound{Msg: strings.Trim(string(msg), "\n")}
+		}
+
+		if res.StatusCode == http.StatusServiceUnavailable {
+			return &ErrRemoteCall{Msg: strings.Trim(string(msg), "\n")}
+		}
+
 		return fmt.Errorf("server returned: %v, %v", res.Status, string(msg))
 	}
 	b := bufferPool.Get().(*bytes.Buffer)

http_test.go

@@ -33,6 +33,8 @@ import (
 	"sync"
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/assert"
 )
 
 var (
@@ -183,6 +185,24 @@ func TestHTTPPool(t *testing.T) {
 	if suffix := ":" + key; !strings.HasSuffix(value, suffix) {
 		t.Errorf("Get(%q) = %q, want value ending in %q", key, value, suffix)
 	}
+
+	// Get a key that does not exist
+	err := g.Get(ctx, "IDoNotExist", StringSink(&value))
+	errNotFound := &ErrNotFound{}
+	if !errors.As(err, &errNotFound) {
+		t.Fatal(errors.New("expected error to be 'ErrNotFound'"))
+	}
+	assert.Equal(t, "I do not exist error", errNotFound.Error())
+
+	// Get a key that is guaranteed to return an error
+	err = g.Get(ctx, "IAlwaysReturnAnError", StringSink(&value))
+	errRemoteCall := &ErrRemoteCall{}
+	if !errors.As(err, &errRemoteCall) {
+		t.Fatal(errors.New("expected error to be 'ErrRemoteCall'"))
+	}
+
+	assert.Equal(t, "I am a GetterFunc error", errRemoteCall.Error())
+
 }
 
 func testKeys(n int) (keys []string) {
@@ -200,6 +220,14 @@ func beChildForTestHTTPPool(t *testing.T) {
 	p.Set(addrToURL(addrs)...)
 
 	getter := GetterFunc(func(ctx context.Context, key string, dest Sink) error {
+		if key == "IDoNotExist" {
+			return &ErrNotFound{Msg: "I do not exist error"}
+		}
+
+		if key == "IAlwaysReturnAnError" {
+			return &ErrRemoteCall{Msg: "I am a GetterFunc error"}
+		}
+
 		if _, err := http.Get(*serverAddr); err != nil {
 			t.Logf("HTTP request from getter failed with '%s'", err)
 		}