Merge pull request hashicorp#315 from hashicorp/fix-req-race-condition

HttpGetter: Avoid reusing the same http.Request
julian7 · Mar 31, 2021 · 5b1298a · 5b1298a
2 parents 8724413 + 3cca945
commit 5b1298a
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 6 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -15,14 +15,17 @@ commands:
         type: string
       platform:
         type: string
+      govet:
+        type: string
+        default: ""
     steps:
       - run:
           name: "Run go tests"
           command: |
             PACKAGE_NAMES=$(go list ./... | circleci tests split --split-by=timings --timings-type=classname)
             echo "Running $(echo $PACKAGE_NAMES | wc -w) packages"
             echo $PACKAGE_NAMES
-            << parameters.cmd >> --format=short-verbose --junitfile $TEST_RESULTS_PATH/go-getter/gotestsum-report.xml -- -p 2 -cover -coverprofile=<< parameters.platform >>_cov_$CIRCLE_NODE_INDEX.part $PACKAGE_NAMES
+            << parameters.cmd >> --format=short-verbose --junitfile $TEST_RESULTS_PATH/go-getter/gotestsum-report.xml -- -p 2 -cover -race -vet=<< parameters.govet >> -coverprofile=<< parameters.platform >>_cov_$CIRCLE_NODE_INDEX.part $PACKAGE_NAMES
         
 jobs:
   linux-tests:
@@ -140,6 +143,9 @@ jobs:
       - run-gotests:
           cmd: "./gotestsum.exe"
           platform: "win"
+          # Otherwise gcc is required for race detector
+          # See https://github.com/golang/go/issues/27089
+          govet: "off"
 
       # Save coverage report parts
       - persist_to_workspace:

diff --git a/get_http.go b/get_http.go
@@ -181,7 +181,6 @@ func (g *HttpGetter) GetFile(dst string, src *url.URL) error {
 				if fi, err := f.Stat(); err == nil {
 					if _, err = f.Seek(0, io.SeekEnd); err == nil {
 						currentFileSize = fi.Size()
-						req.Header.Set("Range", fmt.Sprintf("bytes=%d-", currentFileSize))
 						if currentFileSize >= headResp.ContentLength {
 							// file already present
 							return nil
@@ -191,7 +190,17 @@ func (g *HttpGetter) GetFile(dst string, src *url.URL) error {
 			}
 		}
 	}
-	req.Method = "GET"
+
+	req, err = http.NewRequest("GET", src.String(), nil)
+	if err != nil {
+		return err
+	}
+	if g.Header != nil {
+		req.Header = g.Header.Clone()
+	}
+	if currentFileSize > 0 {
+		req.Header.Set("Range", fmt.Sprintf("bytes=%d-", currentFileSize))
+	}
 
 	resp, err := g.Client.Do(req)
 	if err != nil {

diff --git a/get_http_test.go b/get_http_test.go
@@ -298,6 +298,23 @@ func TestHttpGetter_file(t *testing.T) {
 	assertContents(t, dst, "Hello\n")
 }
 
+// TestHttpGetter_http2server tests that http.Request is not reused
+// between HEAD & GET, which would lead to race condition in HTTP/2.
+// This test is only meaningful for the race detector (go test -race).
+func TestHttpGetter_http2server(t *testing.T) {
+	g := new(HttpGetter)
+	src, err := url.Parse("https://releases.hashicorp.com/terraform/0.14.0/terraform_0.14.0_SHA256SUMS")
+	if err != nil {
+		t.Fatal(err)
+	}
+	dst := tempTestFile(t)
+
+	err = g.GetFile(dst, src)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
 func TestHttpGetter_auth(t *testing.T) {
 	ln := testHttpServer(t)
 	defer ln.Close()

diff --git a/go.mod b/go.mod
@@ -7,7 +7,7 @@ require (
 	github.com/cheggaaa/pb v1.0.27
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/fatih/color v1.7.0 // indirect
-	github.com/hashicorp/go-cleanhttp v0.5.0
+	github.com/hashicorp/go-cleanhttp v0.5.2
 	github.com/hashicorp/go-safetemp v1.0.0
 	github.com/hashicorp/go-version v1.1.0
 	github.com/klauspost/compress v1.11.2

diff --git a/go.sum b/go.sum
@@ -42,8 +42,8 @@ github.com/google/pprof v0.0.0-20190515194954-54271f7e092f/go.mod h1:zfwlbNMJ+OI
 github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+vpHVxEJEs9eg=
 github.com/googleapis/gax-go/v2 v2.0.5 h1:sjZBwGj9Jlw33ImPtvFviGYvseOtDM7hkSKB7+Tv3SM=
 github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
-github.com/hashicorp/go-cleanhttp v0.5.0 h1:wvCrVc9TjDls6+YGAF2hAifE1E5U1+b4tH6KdvN3Gig=
-github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80=
+github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ=
+github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48=
 github.com/hashicorp/go-safetemp v1.0.0 h1:2HR189eFNrjHQyENnQMMpCiBAsRxzbTMIgBhEyExpmo=
 github.com/hashicorp/go-safetemp v1.0.0/go.mod h1:oaerMy3BhqiTbVye6QuFhFtIceqFoDHxNAB65b+Rj1I=
 github.com/hashicorp/go-version v1.1.0 h1:bPIoEKD27tNdebFGGxxYwcL4nepeY4j1QP23PFRGzg0=