Allow HTTP::Client to use ipv6 addresses

[bcardiff]

Fixes #5514

The URI#host is fine to have the brackets since they are part of rfc3986 host description.
The HTTP::Client needs to extract them before creating the TCPSocket.

[j8r]

@host.starts_with?('[') && @host.ends_with?(']') is x2 faster.
@host.lstrip.rstrip is said to be 7x than @host[1..-2]. I wonder if adding a method to strip both at the start and at the end can make sense.

host = "[test]"

Benchmark.ips do |x|
  x.report("index") do
    host[1..-2]
  end

  x.report("strip") do
      host.lstrip.rstrip
  end                              
end

index  29.23M ( 34.21ns) (± 4.66%)  7.72× slower
     strip 225.75M (  4.43ns) (± 3.75%)       fastest

[RX14]

Thats because lstrip doesn't do what you want here? i.e. it doesn't do anything so of course it's faster.

[RX14]

.lchop('[').rchop(']') should be shorter here though. Surely the same speed.

[j8r]

Sorry for the mistake, I confuse the two. Change strip to chop to have lchop.rchop
With chops the program is slower

[RX14]

I feel like this line should be automatic. The glue code to support ipv6 should be in TCPSocket.new or in URI.

[RX14]

Both Ruby and Go expose a hostname, which is just the host without [] if it's ipv6. Perhaps we should copy them.

[bcardiff]

I moved the unwrap to URI, but since in http client @host is String, it needs to be URI.hostname(host : String). But I also added URI#hostname nevertheless.

Regarding benchmark, the original code wins:

require "benchmark"

host = "[test#{Random.new.next_int}]"
pp host
pp host[0] == '[' && host[-1] == ']' ? host[1..-2] : host

Benchmark.ips do |x|
  x.report("index") do
    host[0] == '[' && host[-1] == ']' ? host[1..-2] : host
  end

  x.report("chop") do
    host.lchop('[').rchop(']')
  end
end

$ crystal bench.cr --release
host # => "[test778306676]"
(host[0] == '[') && (host[-1] == ']') ? host[1..-2] : host # => "test778306676"
index  15.24M ( 65.62ns) (± 7.53%)       fastest
 chop  10.08M ( 99.24ns) (± 8.62%)  1.51× slower

[j8r]

@bcardiff Sorry for the mistake. @host.starts_with?('[') && @host.ends_with?(']') seems faster, at least it is on my side.

[RX14]

We can remove the ugly URI.hostname if we move the normalization of @host to the HTTP::Client constructor which takes URI. here

[bcardiff]

@RX14 that won't work. There are initializers that use String and not URI.
But I am ok to mark URI.hostname as :nodoc:.

[RX14]

@bcardiff that's fine - if someone passes HTTP::Client.new("[...]") they can fail. We only care about correct initialization from URI because the [] only come from URI because they're a workaround for URI using : as port number when ipv6 used the same character. If people pass in a hostname string containing brackets, they obviously mean to attempt to resolve that.

[bcardiff]

@RX14 I don't agree. clients initialized with HTTP::Client.new("[::1]") will work. And actually HTTP::Client.new("::1") won't, or at least it is not guaranteed. The host http header are supposed to have a [::1] and not ::1. So the @host needs to have the brackets as in URI.

Otherwise we need to add logic to wrap the host upon client. It can be done, but is a different issue.

Bottom line @hostin HTTP::Client is expected to match URI#host.

[RX14]

Well I really hate URI.hostname, so let's just open-code the removal of the [] into the method, like this was previously.

Then add URI#hostname in this or another PR.

[bcardiff]

But that would lead to two implementation of the same thing. Let's leave URI.hostname as :nodoc: and already have URI#hostname.

I'm satisfied with the state of the PR.

[RX14]

A bit of copy and paste really isn't that bad, especially for a snippet of code so small and unlikely to change as this. DRY shouldn't be taken to extremes, and I think URI.hostname is doing that.

[ysbaddaden]

I agree. Copying once is okayish. Copying a second time should trigger a refactor, but once is acceptable.

[bcardiff]

Done, the PR should be squashed on merge. (I have the feeling that URI.hostname will come back sooner that later, but ok)