From cac00f64df8667153492ffa30f816eeed8038891 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 5 Feb 2019 11:57:57 +0100 Subject: [PATCH 1/8] checksumFromFile: pass context and progress listener --- checksum.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/checksum.go b/checksum.go index 61df54043..9d886a1f3 100644 --- a/checksum.go +++ b/checksum.go @@ -179,13 +179,15 @@ func (c *Client) checksumFromFile(checksumFile string, src *url.URL) (*fileCheck defer os.Remove(tempfile) c2 := &Client{ - Getters: c.Getters, - Decompressors: c.Decompressors, - Detectors: c.Detectors, - Pwd: c.Pwd, - Dir: false, - Src: checksumFile, - Dst: tempfile, + Ctx: c.Ctx, + Getters: c.Getters, + Decompressors: c.Decompressors, + Detectors: c.Detectors, + Pwd: c.Pwd, + Dir: false, + Src: checksumFile, + Dst: tempfile, + ProgressListener: c.ProgressListener, } if err = c2.Get(); err != nil { return nil, fmt.Errorf( From f645fc4c71ce7a0c511cd7decd7f23d063beee87 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 5 Feb 2019 12:35:52 +0100 Subject: [PATCH 2/8] HttpGetter.GetFile: defer close the file so that we don't leak a fd in case there's an error --- get_http.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/get_http.go b/get_http.go index ae04f27f2..01fb6ae09 100644 --- a/get_http.go +++ b/get_http.go @@ -128,7 +128,7 @@ func (g *HttpGetter) Get(dst string, u *url.URL) error { return g.getSubdir(ctx, dst, source, subDir) } -func (g *HttpGetter) GetFile(dst string, src *url.URL) error { +func (g *HttpGetter) GetFile(dst string, src *url.URL) (err error) { ctx := g.Context() if g.Netrc { // Add auth from netrc if we can @@ -146,6 +146,12 @@ func (g *HttpGetter) GetFile(dst string, src *url.URL) error { if err != nil { return err } + defer func() { + cerr := f.Close() + if err == nil { + err = cerr + } + }() if g.Client == nil { g.Client = httpClient @@ -211,9 +217,6 @@ func (g *HttpGetter) GetFile(dst string, src *url.URL) error { if err == nil && n < resp.ContentLength { err = io.ErrShortWrite } - if err1 := f.Close(); err == nil { - err = err1 - } return err } From c2d455ed7f4cfa5f9d8387792706cee9e1c6d247 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Thu, 7 Feb 2019 16:41:24 +0100 Subject: [PATCH 3/8] test download restart + checksumming Apparently when we finish downloading a file and checksum right after checksumming fails but if we run a go-get after this, it works. Making this test more thorough showcases this. --- get_http_test.go | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/get_http_test.go b/get_http_test.go index 1ba60a0f8..9ec1c707d 100644 --- a/get_http_test.go +++ b/get_http_test.go @@ -1,6 +1,8 @@ package getter import ( + "crypto/sha256" + "encoding/hex" "errors" "fmt" "io/ioutil" @@ -167,12 +169,16 @@ func TestHttpGetter_none(t *testing.T) { func TestHttpGetter_resume(t *testing.T) { load := []byte(testHttpMetaStr) + sha := sha256.New() + if n, err := sha.Write(load); n != len(load) || err != nil { + t.Fatalf("sha write failed: %d, %s", n, err) + } + checksum := hex.EncodeToString(sha.Sum(nil)) downloadFrom := len(load) / 2 ln := testHttpServer(t) defer ln.Close() - g := new(HttpGetter) dst := tempDir(t) defer os.RemoveAll(dst) @@ -181,28 +187,39 @@ func TestHttpGetter_resume(t *testing.T) { if err != nil { t.Fatalf("create: %v", err) } - f.Write(load[:downloadFrom]) - f.Close() + if n, err := f.Write(load[:downloadFrom]); n != downloadFrom || err != nil { + t.Fatalf("partia file write failed: %d, %s", n, err) + } + if err := f.Close(); err != nil { + t.Fatalf("close failed: %s", err) + } - var u url.URL - u.Scheme = "http" - u.Host = ln.Addr().String() - u.Path = "/range" + u := url.URL{ + Scheme: "http", + Host: ln.Addr().String(), + Path: "/range", + RawQuery: "checksum=" + checksum, + } + t.Logf("url: %s", u.String()) - // Get it! - if err := g.GetFile(dst, &u); err != nil { - t.Fatalf("should not error: %v", err) + // Finish getting it! + if err := GetFile(dst, u.String()); err != nil { + t.Errorf("finishing download should not error: %v", err) } b, err := ioutil.ReadFile(dst) if err != nil { - t.Fatalf("should not error: %v", err) + t.Fatalf("readfile failed: %v", err) } if string(b) != string(load) { - t.Fatalf("file differs: got:\n%s\n expected:\n%s\n", string(b), string(load)) + t.Errorf("file differs: got:\n%s\n expected:\n%s\n", string(b), string(load)) } + // Get it again + if err := GetFile(dst, u.String()); err != nil { + t.Errorf("should not error: %v", err) + } } func TestHttpGetter_file(t *testing.T) { From dd24b919584a6a71bf7d73a0c48e8d7d54a6ef7e Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Thu, 7 Feb 2019 17:32:40 +0100 Subject: [PATCH 4/8] file checksum: reset hash before checksumming a file Because before downloading the file we checksum it to see if it matches in order to skip the download. --- checksum.go | 1 + 1 file changed, 1 insertion(+) diff --git a/checksum.go b/checksum.go index 9d886a1f3..ef27951b9 100644 --- a/checksum.go +++ b/checksum.go @@ -36,6 +36,7 @@ func (c *fileChecksum) checksum(source string) error { } defer f.Close() + c.Hash.Reset() if _, err := io.Copy(c.Hash, f); err != nil { return fmt.Errorf("Failed to hash: %s", err) } From 6f58df6b8eec1f86866fb9c1a5193a14c982beeb Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Thu, 7 Feb 2019 18:06:27 +0100 Subject: [PATCH 5/8] get_http_test.go: fix log typo --- get_http_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/get_http_test.go b/get_http_test.go index 9ec1c707d..b089c4ab7 100644 --- a/get_http_test.go +++ b/get_http_test.go @@ -188,7 +188,7 @@ func TestHttpGetter_resume(t *testing.T) { t.Fatalf("create: %v", err) } if n, err := f.Write(load[:downloadFrom]); n != downloadFrom || err != nil { - t.Fatalf("partia file write failed: %d, %s", n, err) + t.Fatalf("partial file write failed: %d, %s", n, err) } if err := f.Close(); err != nil { t.Fatalf("close failed: %s", err) From 9d52b634d80f97dfa7687e30cd59a3971a5b773c Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Thu, 7 Feb 2019 18:13:08 +0100 Subject: [PATCH 6/8] TestHttpGetter_resume: fatal if files are different. --- get_http_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/get_http_test.go b/get_http_test.go index b089c4ab7..cbceb1152 100644 --- a/get_http_test.go +++ b/get_http_test.go @@ -213,7 +213,7 @@ func TestHttpGetter_resume(t *testing.T) { } if string(b) != string(load) { - t.Errorf("file differs: got:\n%s\n expected:\n%s\n", string(b), string(load)) + t.Fatalf("file differs: got:\n%s\n expected:\n%s\n", string(b), string(load)) } // Get it again From f0a5d0dfe84197314e6904fcae9b77169da4d81b Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Thu, 7 Feb 2019 18:18:44 +0100 Subject: [PATCH 7/8] ignore the close error as it's in a defer * this avoids naming the error in the func Api, and makes the function simpler in general --- get_http.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/get_http.go b/get_http.go index 01fb6ae09..7c4541c6e 100644 --- a/get_http.go +++ b/get_http.go @@ -128,7 +128,7 @@ func (g *HttpGetter) Get(dst string, u *url.URL) error { return g.getSubdir(ctx, dst, source, subDir) } -func (g *HttpGetter) GetFile(dst string, src *url.URL) (err error) { +func (g *HttpGetter) GetFile(dst string, src *url.URL) error { ctx := g.Context() if g.Netrc { // Add auth from netrc if we can @@ -146,12 +146,7 @@ func (g *HttpGetter) GetFile(dst string, src *url.URL) (err error) { if err != nil { return err } - defer func() { - cerr := f.Close() - if err == nil { - err = cerr - } - }() + defer f.Close() if g.Client == nil { g.Client = httpClient From 675f23ebd58b7f9aaf12cc5e4241a9373df59086 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Thu, 7 Feb 2019 18:58:03 +0100 Subject: [PATCH 8/8] TestHttpGetter_resume: Use Fatalf instead of Errorf It is very likely that if one GetFile fails, the other will. Thus polluting test output. after @vancluever's very nice explanation: https://github.com/hashicorp/go-getter/pull/163#discussion_r254797945 Co-Authored-By: Chris Marchesi --- get_http_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/get_http_test.go b/get_http_test.go index cbceb1152..9424e614f 100644 --- a/get_http_test.go +++ b/get_http_test.go @@ -204,7 +204,7 @@ func TestHttpGetter_resume(t *testing.T) { // Finish getting it! if err := GetFile(dst, u.String()); err != nil { - t.Errorf("finishing download should not error: %v", err) + t.Fatalf("finishing download should not error: %v", err) } b, err := ioutil.ReadFile(dst) @@ -218,7 +218,7 @@ func TestHttpGetter_resume(t *testing.T) { // Get it again if err := GetFile(dst, u.String()); err != nil { - t.Errorf("should not error: %v", err) + t.Fatalf("should not error: %v", err) } }