Compare commits

...

2 Commits

Author SHA1 Message Date
ct16k
a8d6943162
Merge 7f8db4d8ed into 9f417fbc4f 2023-06-12 16:41:25 -04:00
Derrick J. Wippler
9f417fbc4f
Improve HTTP error handling (#60) 2023-06-12 12:04:46 -05:00
5 changed files with 92 additions and 9 deletions

View File

@ -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

33
errors.go Normal file
View File

@ -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
}

View File

@ -427,8 +427,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
}
@ -447,10 +456,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)

19
http.go
View File

@ -19,6 +19,7 @@ package groupcache
import (
"bytes"
"context"
"errors"
"fmt"
"io"
"io/ioutil"
@ -238,7 +239,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
}
@ -321,7 +326,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)

View File

@ -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)
}