Repository: buchgr/bazel-remote
Pull request #325 (2020-07-31 10:17:05)

cache.Proxy.Put should take an io.ReadCloser instead of io.Reader
We pass an *io.File to the cache.Proxy.Put, which should be closed.


Patch:
diff --git a/cache/cache.go b/cache/cache.go
index 68dbf06..e1575db 100644
--- a/cache/cache.go
+++ b/cache/cache.go
@@ -52,7 +52,7 @@ func (e *Error) Error() string {
 type Proxy interface {
 	// Put should make a reasonable effort to proxy this data to the backend.
 	// This is allowed to fail silently (eg when under heavy load).
-	Put(kind EntryKind, hash string, size int64, rdr io.Reader)
+	Put(kind EntryKind, hash string, size int64, rdr io.ReadCloser)
 
 	// Get should return the cache item identified by `hash`, or an error
 	// if something went wrong. If the item was not found, the io.ReadCloser
diff --git a/cache/httpproxy/httpproxy.go b/cache/httpproxy/httpproxy.go
index 01326f8..368ff49 100644
--- a/cache/httpproxy/httpproxy.go
+++ b/cache/httpproxy/httpproxy.go
@@ -24,7 +24,7 @@ type uploadReq struct {
 	hash string
 	size int64
 	kind cache.EntryKind
-	rdr  io.Reader
+	rc   io.ReadCloser
 }
 
 type remoteHTTPProxyCache struct {
@@ -50,8 +50,9 @@ func uploadFile(remote *http.Client, baseURL *url.URL, accessLogger cache.Logger
 	errorLogger cache.Logger, item uploadReq) {
 
 	if item.size == 0 {
+		item.rc.Close()
 		// See https://github.com/golang/go/issues/20257#issuecomment-299509391
-		item.rdr = http.NoBody
+		item.rc = http.NoBody
 	}
 
 	url := requestURL(baseURL, item.hash, item.kind)
@@ -62,8 +63,12 @@ func uploadFile(remote *http.Client, baseURL *url.URL, accessLogger cache.Logger
 		return
 	}
 
-	req, err := http.NewRequest(http.MethodPut, url, item.rdr)
+	req, err := http.NewRequest(http.MethodPut, url, item.rc)
 	if err != nil {
+		// item.rc will be closed if we call req.Do(), but not if we
+		// return earlier.
+		item.rc.Close()
+
 		return
 	}
 	req.Header.Set("Content-Type", "application/octet-stream")
@@ -109,16 +114,17 @@ func logResponse(logger cache.Logger, method string, code int, url string) {
 	logger.Printf("HTTP %s %d %s", method, code, url)
 }
 
-func (r *remoteHTTPProxyCache) Put(kind cache.EntryKind, hash string, size int64, rdr io.Reader) {
+func (r *remoteHTTPProxyCache) Put(kind cache.EntryKind, hash string, size int64, rc io.ReadCloser) {
 	select {
 	case r.uploadQueue <- uploadReq{
 		hash: hash,
 		size: size,
 		kind: kind,
-		rdr:  rdr,
+		rc:   rc,
 	}:
 	default:
 		r.errorLogger.Printf("too many uploads queued")
+		rc.Close()
 	}
 }
 
diff --git a/cache/s3proxy/s3proxy.go b/cache/s3proxy/s3proxy.go
index e74a061..b361c74 100644
--- a/cache/s3proxy/s3proxy.go
+++ b/cache/s3proxy/s3proxy.go
@@ -22,7 +22,7 @@ type uploadReq struct {
 	hash string
 	size int64
 	kind cache.EntryKind
-	rdr  io.Reader
+	rc   io.ReadCloser
 }
 
 type s3Cache struct {
@@ -139,7 +139,7 @@ func (c *s3Cache) uploadFile(item uploadReq) {
 		context.Background(),
 		c.bucket,                          // bucketName
 		c.objectKey(item.hash, item.kind), // objectName
-		item.rdr,                          // reader
+		item.rc,                           // reader
 		item.size,                         // objectSize
 		"",                                // md5base64
 		uploadDigest,                      // sha256
@@ -151,18 +151,21 @@ func (c *s3Cache) uploadFile(item uploadReq) {
 	)
 
 	logResponse(c.accessLogger, "UPLOAD", c.bucket, c.objectKey(item.hash, item.kind), err)
+
+	item.rc.Close()
 }
 
-func (c *s3Cache) Put(kind cache.EntryKind, hash string, size int64, rdr io.Reader) {
+func (c *s3Cache) Put(kind cache.EntryKind, hash string, size int64, rc io.ReadCloser) {
 	select {
 	case c.uploadQueue <- uploadReq{
 		hash: hash,
 		size: size,
 		kind: kind,
-		rdr:  rdr,
+		rc:   rc,
 	}:
 	default:
 		c.errorLogger.Printf("too many uploads queued\n")
+		rc.Close()
 	}
 }
 


Test patch:
diff --git a/cache/disk/disk_test.go b/cache/disk/disk_test.go
index 646bb35..bcb3a64 100644
--- a/cache/disk/disk_test.go
+++ b/cache/disk/disk_test.go
@@ -94,7 +94,8 @@ func TestCacheBasics(t *testing.T) {
 	}
 
 	// Add an item
-	err = testCache.Put(cache.CAS, contentsHash, int64(len(contents)), strings.NewReader(contents))
+	err = testCache.Put(cache.CAS, contentsHash, int64(len(contents)),
+		ioutil.NopCloser(strings.NewReader(contents)))
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -147,7 +148,8 @@ func TestCacheEviction(t *testing.T) {
 				sha256.Size*2, len(key), key)
 		}
 
-		err := testCache.Put(cache.AC, key, int64(i), strReader)
+		err := testCache.Put(cache.AC, key, int64(i),
+			ioutil.NopCloser(strReader))
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -261,7 +263,7 @@ func TestCacheGetContainsWrongSizeWithProxy(t *testing.T) {
 // digest {contentsHash, contentsLength}.
 type proxyStub struct{}
 
-func (d proxyStub) Put(kind cache.EntryKind, hash string, size int64, rdr io.Reader) {}
+func (d proxyStub) Put(kind cache.EntryKind, hash string, size int64, rc io.ReadCloser) {}
 
 func (d proxyStub) Get(kind cache.EntryKind, hash string) (io.ReadCloser, int64, error) {
 	if hash != contentsHash {