Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use Response.ContentLength & clarify log messages #49

Merged
merged 1 commit into from Feb 22, 2022
Merged

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Feb 3, 2022

I spent time wondering why does checksum calculation take a while until I realized what happened.

2022/02/03 16:35:05 will install into dir at /var/folders/9w/pddq2x656j76vxvsbhcs4nwr0000gq/T/TestRepro2318612991/001/hcinstall
2022/02/03 16:35:05 requesting versions from https://releases.hashicorp.com/terraform/index.json
2022/02/03 16:35:05 received 200 OK
2022/02/03 16:35:05 downloading signature from https://releases.hashicorp.com/terraform/1.1.5/terraform_1.1.5_SHA256SUMS.72D7468F.sig
2022/02/03 16:35:05 downloading checksums from https://releases.hashicorp.com/terraform/1.1.5/terraform_1.1.5_SHA256SUMS
2022/02/03 16:35:05 checksum signature is valid
2022/02/03 16:35:05 downloading archive from https://releases.hashicorp.com/terraform/1.1.5/terraform_1.1.5_darwin_arm64.zip
2022/02/03 16:35:05 calculating checksum of "terraform_1.1.5_darwin_arm64.zip"
2022/02/03 16:35:56 checksum matches: "723363af9524c0897e9a7d871d27f0d96f6aafd11990df7e6348f5b45d2dbe2c"
2022/02/03 16:35:56 copying downloaded file to /var/folders/9w/pddq2x656j76vxvsbhcs4nwr0000gq/T/terraform_1.1.5_darwin_arm64.zip4179759653
2022/02/03 16:35:56 copied 19328643 bytes to /var/folders/9w/pddq2x656j76vxvsbhcs4nwr0000gq/T/terraform_1.1.5_darwin_arm64.zip4179759653
2022/02/03 16:35:56 unpacking terraform to /var/folders/9w/pddq2x656j76vxvsbhcs4nwr0000gq/T/TestRepro2318612991/001/hcinstall
2022/02/03 16:35:56 changing perms of /var/folders/9w/pddq2x656j76vxvsbhcs4nwr0000gq/T/TestRepro2318612991/001/hcinstall/terraform

The first time we call io.Copy() - it is to read the bytes from http.Response.Body and write them into sha256 hash.Hash and a bytes.Buffer (via TeeReader) for later copying into the destination file (if checksum matches). Now http.Response.Body does not buffer the bytes internally (which is a very sensible design decision), instead it streams them on-demand over the network. If the network is slow then this may therefore take a while.

https://pkg.go.dev/net/http#Response

type Response struct {
	// Body represents the response body.
	//
	// The response body is streamed on demand as the Body field
	// is read. If the network connect[io](https://pkg.go.dev/io)n fails or the server
	// terminates the response, Body.Read calls return an error.
	// ...
	Body io.[ReadCloser](https://pkg.go.dev/io#ReadCloser)

I originally intended the relevant code to avoid buffering - which it does - but only in cases where checksum verification is skipped, which arguably should be minority of cases.

I am not sure we can avoid buffering and avoid downloading the bytes twice (before and after verification), so retrospectively it's possible this may have been over-optimization and for code readability it would probably be beneficial to just always write into a buffer and pass it around? I definitely want to avoid writing the bytes into a file on the filesystem prior to having a chance to verify checksum but keeping it in a buffer seems harmless.

I'm open to any other thoughts/suggestions.

@radeksimko radeksimko requested a review from kmoe Feb 3, 2022
kmoe
kmoe approved these changes Feb 22, 2022
@radeksimko radeksimko merged commit 1c40a7d into main Feb 22, 2022
5 checks passed
@radeksimko radeksimko deleted the clarify-logs branch Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants