search

willnorris / webmention

Go library and CLI for assisting in sending webmentions.
https://pkg.go.dev/willnorris.com/go/webmention
BSD 3-Clause "New" or "Revised" License
50 stars 5 forks source link

feat: try HEAD request before GET #16

Closed hacdias closed 3 years ago

hacdias commented 3 years ago

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

hacdias commented 3 years ago

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

willnorris commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

well, somewhat hilariously, I just found https://github.com/google/go-github/pull/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 https://github.com/google/go-github/issues/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!