Remove CancelRequest implementation, tracking of request copies.

In older versions of Go and net/http standard library, implementations
of http.RoundTripper had to implement CancelRequest method in order to
enable request cancellation. That CancelRequest method has since been
deprecated in favor of more modern ways of request cancellation, which
is via Request.Cancel field and/or request context. Modern http.Client
uses Request.Cancel field to implement its Timeout field, but also calls
the deprecated CancelRequest method, if it's implemented, just in case.

Since this package is expected to run on modern versions of Go (it
requires Go 1.6+ at this time), we can safely remove the deprecated
CancelRequest method implementation.

The benefits of doing that include:

-	Remove the unneeded "httpcache: Client Transport of type %T doesn't
	support CancelRequest; Timeout not supported" warning from being
	logged.

-	Simplify and remove code for tracking the mapping between the
	original and copied request, which was only needed by the
	implementation of the CancelRequest method.

-	onEOFReader also becomes unused, and can be removed.

Add a simple timeout test to verify that http.Client.Timeout value is
respected when a cache transport is used.

Skip this test in short mode, because it takes at least 3 seconds to
run, to test timeouts. This is significantly longer than other tests.

Resolves #61.
Updates #62.

Author: dmitshur

httpcache.go

@@ -11,8 +11,6 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"io"
-	"log"
 	"net/http"
 	"net/http/httputil"
 	"strings"
@@ -90,33 +88,6 @@ func NewMemoryCache() *MemoryCache {
 	return c
 }
 
-// onEOFReader executes a function on reader EOF or close
-type onEOFReader struct {
-	rc io.ReadCloser
-	fn func()
-}
-
-func (r *onEOFReader) Read(p []byte) (n int, err error) {
-	n, err = r.rc.Read(p)
-	if err == io.EOF {
-		r.runFunc()
-	}
-	return
-}
-
-func (r *onEOFReader) Close() error {
-	err := r.rc.Close()
-	r.runFunc()
-	return err
-}
-
-func (r *onEOFReader) runFunc() {
-	if fn := r.fn; fn != nil {
-		fn()
-		r.fn = nil
-	}
-}
-
 // Transport is an implementation of http.RoundTripper that will return values from a cache
 // where possible (avoiding a network request) and will additionally add validators (etag/if-modified-since)
 // to repeated requests allowing servers to return 304 / Not Modified
@@ -127,10 +98,6 @@ type Transport struct {
 	Cache     Cache
 	// If true, responses returned from the cache will be given an extra header, X-From-Cache
 	MarkCachedResponses bool
-	// guards modReq
-	mu sync.RWMutex
-	// Mapping of original request => cloned
-	modReq map[*http.Request]*http.Request
 }
 
 // NewTransport returns a new Transport with the
@@ -156,20 +123,6 @@ func varyMatches(cachedResp *http.Response, req *http.Request) bool {
 	return true
 }
 
-// setModReq maintains a mapping between original requests and their associated cloned requests
-func (t *Transport) setModReq(orig, mod *http.Request) {
-	t.mu.Lock()
-	if t.modReq == nil {
-		t.modReq = make(map[*http.Request]*http.Request)
-	}
-	if mod == nil {
-		delete(t.modReq, orig)
-	} else {
-		t.modReq[orig] = mod
-	}
-	t.mu.Unlock()
-}
-
 // RoundTrip takes a Request and returns a Response
 //
 // If there is a fresh Response already in cache, then it will be returned without connecting to
@@ -222,22 +175,6 @@ func (t *Transport) RoundTrip(req *http.Request) (resp *http.Response, err error
 					req2.Header.Set("if-modified-since", lastModified)
 				}
 				if req2 != nil {
-					// Associate original request with cloned request so we can refer to
-					// it in CancelRequest(). Release the mapping when it's no longer needed.
-					t.setModReq(req, req2)
-					defer func(originalReq *http.Request) {
-						// Release req/clone mapping on error
-						if err != nil {
-							t.setModReq(originalReq, nil)
-						}
-						if resp != nil {
-							// Release req/clone mapping on body close/EOF
-							resp.Body = &onEOFReader{
-								rc: resp.Body,
-								fn: func() { t.setModReq(originalReq, nil) },
-							}
-						}
-					}(req)
 					req = req2
 				}
 			}
@@ -300,31 +237,6 @@ func (t *Transport) RoundTrip(req *http.Request) (resp *http.Response, err error
 	return resp, nil
 }
 
-// CancelRequest calls CancelRequest on the underlaying transport if implemented or
-// throw a warning otherwise.
-func (t *Transport) CancelRequest(req *http.Request) {
-	type canceler interface {
-		CancelRequest(*http.Request)
-	}
-	tr, ok := t.Transport.(canceler)
-	if !ok {
-		log.Printf("httpcache: Client Transport of type %T doesn't support CancelRequest; Timeout not supported", t.Transport)
-		return
-	}
-
-	t.mu.RLock()
-	if modReq, ok := t.modReq[req]; ok {
-		t.mu.RUnlock()
-		t.mu.Lock()
-		delete(t.modReq, req)
-		t.mu.Unlock()
-		tr.CancelRequest(modReq)
-	} else {
-		t.mu.RUnlock()
-		tr.CancelRequest(req)
-	}
-}
-
 // ErrNoDateHeader indicates that the HTTP headers contained no Date header.
 var ErrNoDateHeader = errors.New("no Date header")
 

httpcache_test.go

@@ -129,6 +129,11 @@ func setup() {
 		}
 		w.Write([]byte("Some text content"))
 	}))
+
+	// Take 3 seconds to return 200 OK (for testing client timeouts).
+	mux.HandleFunc("/3seconds", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(3 * time.Second)
+	}))
 }
 
 func teardown() {
@@ -465,11 +470,6 @@ func TestGetWithLastModified(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer func() {
-		if len(s.transport.modReq) != 0 {
-			t.Errorf("Request-map is not empty")
-		}
-	}()
 	{
 		resp, err := s.client.Do(req)
 		if err != nil {
@@ -1211,3 +1211,31 @@ func TestStaleIfErrorResponseLifetime(t *testing.T) {
 		t.Fatalf("got err %v, want %v", err, tmock.err)
 	}
 }
+
+// Test that http.Client.Timeout is respected when cache transport is used.
+// That is so as long as request cancellation is propagated correctly.
+// In the past, that required CancelRequest to be implemented correctly,
+// but modern http.Client uses Request.Cancel (or request context) instead,
+// so we don't have to do anything.
+func TestClientTimeout(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping timeout test in short mode") // Because it takes at least 3 seconds to run.
+	}
+	resetTest()
+	client := &http.Client{
+		Transport: NewMemoryCacheTransport(),
+		Timeout:   time.Second,
+	}
+	started := time.Now()
+	resp, err := client.Get(s.server.URL + "/3seconds")
+	taken := time.Since(started)
+	if err == nil {
+		t.Error("got nil error, want timeout error")
+	}
+	if resp != nil {
+		t.Error("got non-nil resp, want nil resp")
+	}
+	if taken >= 2*time.Second {
+		t.Error("client.Do took 2+ seconds, want < 2 seconds")
+	}
+}