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

feat: try HEAD request before GET #16

Merged
merged 2 commits into from
Oct 28, 2021
Merged

feat: try HEAD request before GET #16

merged 2 commits into from
Oct 28, 2021

Conversation

hacdias
Copy link
Contributor

@hacdias hacdias commented Oct 28, 2021

I added the HEAD request first.

I also made sure the body is discarded before closing the connection so the http.Client can reuse the TCP connections. I heard this is needed, but I found contradicting information...

Closes #5

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias changed the title feat: do HEAD request before GET feat: try HEAD request before GET Oct 28, 2021
@hacdias
Copy link
Contributor Author

hacdias commented Oct 28, 2021

@willnorris I see the CI fails because io.Discard does not exist in Go 1.15. I can either:

  • Remove that completely;
  • Change to ioutil.Discard;
  • Or change to Go 1.16 on CI because it says "support the two most recent major go versions" and Go 1.15 is not one of the last two.

@willnorris
Copy link
Owner

I don't want break it from working in go1.15 if we don't have to, even if it's not technically in the support window of two latest releases. I'm currently deep into the source code for net/http trying to figure out if we need to close the body ourselves :)

The response body certainly it needs to be read to completion... the docs are pretty clear on that. What I haven't yet found is whether the standard resp.Body.Close() call does that for us. This comment suggests that it does, but I'm not 100% sure that the linked body struct is what is being used by http.Response. Let me keep digging a bit more.

@willnorris
Copy link
Owner

Okay, so http.Response is definitely using that body struct. Everything I'm finding in the net/http source suggests that would shouldn't have to do this manually. There is a ton of logic to try and be really intelligent about reading the full body specifically so that connections can be reused.

I don't think discarding the body like this is harmful in any way, since it's basically what resp.Body.Close() is going to end up doing anyway, but I'd still rather not put it in there unless we have a clearer indication that it's needed (since everything I'm finding suggests that it's not)

@hacdias
Copy link
Contributor Author

hacdias commented Oct 28, 2021

Okay, I removed it then. I found contradicting information from many sources so I'm also quite curious about what's actually happening behind the scenes.

@willnorris
Copy link
Owner

well, somewhat hilariously, I just found google/go-github#317 and remembered that I dealt with this exact problem 5 years ago there :)

That's probably the best source of info on what's going on, and includes some background from bradfitz. And interestingly, we did decide to always drain in that case, though it was later removed as unnecessary in google/go-github#843. because it had gotten fixed in the stdlib.

So yeah, I think leaving this out is the right call until there is specific indication that it's necessary.

Thanks for this patch!

@willnorris willnorris merged commit 7d22f9f into willnorris:main Oct 28, 2021
@hacdias hacdias deleted the ffeat/head-first branch October 28, 2021 17:18
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.

try HEAD before GET for endpoint discovery
2 participants