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>

Author: toothrot

cache.go

@@ -11,6 +11,14 @@ import (
 	"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.

go.mod

@@ -5,6 +5,7 @@ go 1.12
 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

main.go

@@ -39,6 +39,7 @@ func main() {
 			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

sandbox.go

@@ -50,14 +50,16 @@ const (
 	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 @@ func (s *server) commandHandler(cachePrefix string, cmdFunc func(context.Context
 		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 @@ func (s *server) commandHandler(cachePrefix string, cmdFunc func(context.Context
 				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 @@ func (s *server) commandHandler(cachePrefix string, cmdFunc func(context.Context
 				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 @@ func sandboxRun(ctx context.Context, exePath string, testParam string) (sandboxt
 	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)

server.go

@@ -18,7 +18,7 @@ type server struct {
 	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

server_test.go

@@ -7,12 +7,17 @@ package main
 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 @@ func TestCommandHandler(t *testing.T) {
 		// 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 @@ func TestCommandHandler(t *testing.T) {
 		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 @@ func TestPlaygroundGoproxy(t *testing.T) {
 		})
 	}
 }
+
+// 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
+}