From 9f417fbc4f99eb58e51f8be01b1ac627a83a348f Mon Sep 17 00:00:00 2001 From: "Derrick J. Wippler" Date: Mon, 12 Jun 2023 12:04:46 -0500 Subject: [PATCH] Improve HTTP error handling (#60) --- CHANGELOG | 4 +++- errors.go | 33 +++++++++++++++++++++++++++++++++ groupcache.go | 17 +++++++++++------ http.go | 19 +++++++++++++++++-- http_test.go | 28 ++++++++++++++++++++++++++++ 5 files changed, 92 insertions(+), 9 deletions(-) create mode 100644 errors.go diff --git a/CHANGELOG b/CHANGELOG index a9234a0..af5e18e 100644 --- a/CHANGELOG +++ b/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 diff --git a/errors.go b/errors.go new file mode 100644 index 0000000..fdc6a9a --- /dev/null +++ b/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 +} diff --git a/groupcache.go b/groupcache.go index 3d1ebec..541e89a 100644 --- a/groupcache.go +++ b/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) diff --git a/http.go b/http.go index 9a5a6d2..32e1f90 100644 --- a/http.go +++ b/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) diff --git a/http_test.go b/http_test.go index c7b7c20..30f02c4 100644 --- a/http_test.go +++ b/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) }