playground: stop storing timeouts in cache
This changes the Playground to stop storing build and run timeout
responses in the cache. These responses could get cached when the
Playground was unhealthy, leaving some trivial snippets to be cached
incorrectly, confusing users.
Adds testing for our caching logic.
Updates golang/go#38546
Updates golang/go#38576
Change-Id: Idd2106d673162d9eea8536fe2433f74c23ed6e8a
Reviewed-on: https://go-review.googlesource.com/c/playground/+/229307
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
diff --git a/cache.go b/cache.go
index 5d725ba..dca1d23 100644
--- a/cache.go
+++ b/cache.go
@@ -11,6 +11,14 @@
"github.com/bradfitz/gomemcache/memcache"
)
+// responseCache is a common interface for cache implementations.
+type responseCache interface {
+ // Set sets the value for a key.
+ Set(key string, v interface{}) error
+ // Get sets v to the value stored for a key.
+ Get(key string, v interface{}) error
+}
+
// gobCache stores and retrieves values using a memcache client using the gob
// encoding package. It does not currently allow for expiration of items.
// With a nil gobCache, Set is a no-op and Get will always return memcache.ErrCacheMiss.
diff --git a/go.mod b/go.mod
index 5985e8d..b5c0073 100644
--- a/go.mod
+++ b/go.mod
@@ -5,6 +5,7 @@
require (
cloud.google.com/go v0.38.0
github.com/bradfitz/gomemcache v0.0.0-20190329173943-551aad21a668
+ github.com/google/go-cmp v0.3.0
golang.org/x/build v0.0.0-20190709001953-30c0e6b89ea0
golang.org/x/mod v0.2.0
golang.org/x/tools v0.0.0-20200420001825-978e26b7c37c
diff --git a/main.go b/main.go
index 733a6a0..af9774e 100644
--- a/main.go
+++ b/main.go
@@ -39,6 +39,7 @@
s.cache = newGobCache(caddr)
log.Printf("App (project ID: %q) is caching results", pid)
} else {
+ s.cache = (*gobCache)(nil) // Use a no-op cache implementation.
log.Printf("App (project ID: %q) is NOT caching results", pid)
}
s.log = log
diff --git a/sandbox.go b/sandbox.go
index b5d5de9..76fd7fa 100644
--- a/sandbox.go
+++ b/sandbox.go
@@ -50,14 +50,16 @@
progName = "prog.go"
)
-const goBuildTimeoutError = "timeout running go build"
+const (
+ goBuildTimeoutError = "timeout running go build"
+ runTimeoutError = "timeout running program"
+)
-// Responses that contain these strings will not be cached due to
-// their non-deterministic nature.
-var nonCachingErrors = []string{
+// internalErrors are strings found in responses that will not be cached
+// due to their non-deterministic nature.
+var internalErrors = []string{
"out of memory",
"cannot allocate memory",
- goBuildTimeoutError,
}
type request struct {
@@ -115,7 +117,7 @@
resp := &response{}
key := cacheKey(cachePrefix, req.Body)
if err := s.cache.Get(key, resp); err != nil {
- if err != memcache.ErrCacheMiss {
+ if !errors.Is(err, memcache.ErrCacheMiss) {
s.log.Errorf("s.cache.Get(%q, &response): %v", key, err)
}
resp, err = cmdFunc(r.Context(), &req)
@@ -124,12 +126,16 @@
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
return
}
- if strings.Contains(resp.Errors, goBuildTimeoutError) {
- // TODO(golang.org/issue/38052) - This should be a http.StatusBadRequest, but the UI requires a 200 to parse the response.
+ if strings.Contains(resp.Errors, goBuildTimeoutError) || strings.Contains(resp.Errors, runTimeoutError) {
+ // TODO(golang.org/issue/38576) - This should be a http.StatusBadRequest,
+ // but the UI requires a 200 to parse the response. It's difficult to know
+ // if we've timed out because of an error in the code snippet, or instability
+ // on the playground itself. Either way, we should try to show the user the
+ // partial output of their program.
s.writeResponse(w, resp, http.StatusOK)
return
}
- for _, e := range nonCachingErrors {
+ for _, e := range internalErrors {
if strings.Contains(resp.Errors, e) {
s.log.Errorf("cmdFunc compilation error: %q", resp.Errors)
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
@@ -140,7 +146,7 @@
if el.Kind != "stderr" {
continue
}
- for _, e := range nonCachingErrors {
+ for _, e := range internalErrors {
if strings.Contains(el.Message, e) {
s.log.Errorf("cmdFunc runtime error: %q", el.Message)
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
@@ -543,8 +549,8 @@
sreq.GetBody = func() (io.ReadCloser, error) { return ioutil.NopCloser(bytes.NewReader(exeBytes)), nil }
res, err := sandboxBackendClient().Do(sreq)
if err != nil {
- if ctx.Err() == context.DeadlineExceeded {
- execRes.Error = "timeout running program"
+ if errors.Is(ctx.Err(), context.DeadlineExceeded) {
+ execRes.Error = runTimeoutError
return execRes, nil
}
return execRes, fmt.Errorf("POST %q: %w", sandboxBackendURL(), err)
diff --git a/server.go b/server.go
index f091933..9161633 100644
--- a/server.go
+++ b/server.go
@@ -18,7 +18,7 @@
mux *http.ServeMux
db store
log logger
- cache *gobCache
+ cache responseCache
// When the executable was last modified. Used for caching headers of compiled assets.
modtime time.Time
diff --git a/server_test.go b/server_test.go
index a7d9fac..7b6f77b 100644
--- a/server_test.go
+++ b/server_test.go
@@ -7,12 +7,17 @@
import (
"bytes"
"context"
+ "encoding/json"
"fmt"
"io/ioutil"
"net/http"
"net/http/httptest"
"os"
+ "sync"
"testing"
+
+ "github.com/bradfitz/gomemcache/memcache"
+ "github.com/google/go-cmp/cmp"
)
type testLogger struct {
@@ -166,6 +171,7 @@
// Should we verify that s.log.Errorf was called
// instead of just printing or failing the test?
s.log = newStdLogger()
+ s.cache = new(inMemCache)
return nil
})
if err != nil {
@@ -193,63 +199,103 @@
if r.Body == "allocate-memory-compile-error" {
return &response{Errors: "cannot allocate memory"}, nil
}
+ if r.Body == "build-timeout-error" {
+ return &response{Errors: goBuildTimeoutError}, nil
+ }
+ if r.Body == "run-timeout-error" {
+ return &response{Errors: runTimeoutError}, nil
+ }
resp := &response{Events: []Event{{r.Body, "stdout", 0}}}
return resp, nil
})
testCases := []struct {
- desc string
- method string
- statusCode int
- reqBody []byte
- respBody []byte
+ desc string
+ method string
+ statusCode int
+ reqBody []byte
+ respBody []byte
+ shouldCache bool
}{
- {"OPTIONS request", http.MethodOptions, http.StatusOK, nil, nil},
- {"GET request", http.MethodGet, http.StatusBadRequest, nil, nil},
- {"Empty POST", http.MethodPost, http.StatusBadRequest, nil, nil},
- {"Failed cmdFunc", http.MethodPost, http.StatusInternalServerError, []byte(`{"Body":"fail"}`), nil},
+ {"OPTIONS request", http.MethodOptions, http.StatusOK, nil, nil, false},
+ {"GET request", http.MethodGet, http.StatusBadRequest, nil, nil, false},
+ {"Empty POST", http.MethodPost, http.StatusBadRequest, nil, nil, false},
+ {"Failed cmdFunc", http.MethodPost, http.StatusInternalServerError, []byte(`{"Body":"fail"}`), nil, false},
{"Standard flow", http.MethodPost, http.StatusOK,
[]byte(`{"Body":"ok"}`),
[]byte(`{"Errors":"","Events":[{"Message":"ok","Kind":"stdout","Delay":0}],"Status":0,"IsTest":false,"TestsFailed":0}
`),
- },
- {"Errors in response", http.MethodPost, http.StatusOK,
+ true},
+ {"Cache-able Errors in response", http.MethodPost, http.StatusOK,
[]byte(`{"Body":"error"}`),
[]byte(`{"Errors":"errors","Events":null,"Status":0,"IsTest":false,"TestsFailed":0}
`),
- },
+ true},
{"Out of memory error in response body event message", http.MethodPost, http.StatusInternalServerError,
- []byte(`{"Body":"oom-error"}`), nil},
+ []byte(`{"Body":"oom-error"}`), nil, false},
{"Cannot allocate memory error in response body event message", http.MethodPost, http.StatusInternalServerError,
- []byte(`{"Body":"allocate-memory-error"}`), nil},
+ []byte(`{"Body":"allocate-memory-error"}`), nil, false},
{"Out of memory error in response errors", http.MethodPost, http.StatusInternalServerError,
- []byte(`{"Body":"oom-compile-error"}`), nil},
+ []byte(`{"Body":"oom-compile-error"}`), nil, false},
{"Cannot allocate memory error in response errors", http.MethodPost, http.StatusInternalServerError,
- []byte(`{"Body":"allocate-memory-compile-error"}`), nil},
+ []byte(`{"Body":"allocate-memory-compile-error"}`), nil, false},
+ {
+ desc: "Build timeout error",
+ method: http.MethodPost,
+ statusCode: http.StatusOK,
+ reqBody: []byte(`{"Body":"build-timeout-error"}`),
+ respBody: []byte(fmt.Sprintln(`{"Errors":"timeout running go build","Events":null,"Status":0,"IsTest":false,"TestsFailed":0}`)),
+ },
+ {
+ desc: "Run timeout error",
+ method: http.MethodPost,
+ statusCode: http.StatusOK,
+ reqBody: []byte(`{"Body":"run-timeout-error"}`),
+ respBody: []byte(fmt.Sprintln(`{"Errors":"timeout running program","Events":null,"Status":0,"IsTest":false,"TestsFailed":0}`)),
+ },
}
for _, tc := range testCases {
- req := httptest.NewRequest(tc.method, "/compile", bytes.NewReader(tc.reqBody))
- w := httptest.NewRecorder()
- testHandler(w, req)
- resp := w.Result()
- corsHeader := "Access-Control-Allow-Origin"
- if got, want := resp.Header.Get(corsHeader), "*"; got != want {
- t.Errorf("%s: %q header: got %q; want %q", tc.desc, corsHeader, got, want)
- }
- if got, want := resp.StatusCode, tc.statusCode; got != want {
- t.Errorf("%s: got unexpected status code %d; want %d", tc.desc, got, want)
- }
- if tc.respBody != nil {
- defer resp.Body.Close()
- b, err := ioutil.ReadAll(resp.Body)
- if err != nil {
- t.Errorf("%s: ioutil.ReadAll(resp.Body): %v", tc.desc, err)
+ t.Run(tc.desc, func(t *testing.T) {
+ req := httptest.NewRequest(tc.method, "/compile", bytes.NewReader(tc.reqBody))
+ w := httptest.NewRecorder()
+ testHandler(w, req)
+ resp := w.Result()
+ corsHeader := "Access-Control-Allow-Origin"
+ if got, want := resp.Header.Get(corsHeader), "*"; got != want {
+ t.Errorf("%s: %q header: got %q; want %q", tc.desc, corsHeader, got, want)
}
- if !bytes.Equal(b, tc.respBody) {
- t.Errorf("%s: got unexpected body %q; want %q", tc.desc, b, tc.respBody)
+ if got, want := resp.StatusCode, tc.statusCode; got != want {
+ t.Errorf("%s: got unexpected status code %d; want %d", tc.desc, got, want)
}
- }
+ if tc.respBody != nil {
+ defer resp.Body.Close()
+ b, err := ioutil.ReadAll(resp.Body)
+ if err != nil {
+ t.Errorf("%s: ioutil.ReadAll(resp.Body): %v", tc.desc, err)
+ }
+ if !bytes.Equal(b, tc.respBody) {
+ t.Errorf("%s: got unexpected body %q; want %q", tc.desc, b, tc.respBody)
+ }
+ }
+
+ // Test caching semantics.
+ sbreq := new(request) // A sandbox request, used in the cache key.
+ json.Unmarshal(tc.reqBody, sbreq) // Ignore errors, request may be empty.
+ gotCache := new(response)
+ if err := s.cache.Get(cacheKey("test", sbreq.Body), gotCache); (err == nil) != tc.shouldCache {
+ t.Errorf("s.cache.Get(%q, %v) = %v, shouldCache: %v", cacheKey("test", sbreq.Body), gotCache, err, tc.shouldCache)
+ }
+ wantCache := new(response)
+ if tc.shouldCache {
+ if err := json.Unmarshal(tc.respBody, wantCache); err != nil {
+ t.Errorf("json.Unmarshal(%q, %v) = %v, wanted no error", tc.respBody, wantCache, err)
+ }
+ }
+ if diff := cmp.Diff(wantCache, gotCache); diff != "" {
+ t.Errorf("s.Cache.Get(%q) mismatch (-want +got):\n%s", cacheKey("test", sbreq.Body), diff)
+ }
+ })
}
}
@@ -315,3 +361,36 @@
})
}
}
+
+// inMemCache is a responseCache backed by a map. It is only suitable for testing.
+type inMemCache struct {
+ l sync.Mutex
+ m map[string]*response
+}
+
+// Set implements the responseCache interface.
+// Set stores a *response in the cache. It panics for other types to ensure test failure.
+func (i *inMemCache) Set(key string, v interface{}) error {
+ i.l.Lock()
+ defer i.l.Unlock()
+ if i.m == nil {
+ i.m = make(map[string]*response)
+ }
+ i.m[key] = v.(*response)
+ return nil
+}
+
+// Get implements the responseCache interface.
+// Get fetches a *response from the cache, or returns a memcache.ErrcacheMiss.
+// It panics for other types to ensure test failure.
+func (i *inMemCache) Get(key string, v interface{}) error {
+ i.l.Lock()
+ defer i.l.Unlock()
+ target := v.(*response)
+ got, ok := i.m[key]
+ if !ok {
+ return memcache.ErrCacheMiss
+ }
+ *target = *got
+ return nil
+}
