Allow HTTP::Client to use ipv6 addresses #6147

bcardiff · 2018-05-31T08:39:26Z

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 · 2018-05-31T09:09:15Z

src/http/client.cr

+    # URI host may have ipv6 enclosed by brackets and is fine according to
+    # rfc3986. The host http header must contains the brackets, ie: the same
+    # value as returned by URI#host
+    ip_host = @host[0] == '[' && @host[-1] == ']' ? @host[1..-2] : @host


@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

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

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

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

Fixes crystal-lang#5514

RX14 · 2018-05-31T13:24:07Z

src/http/client.cr

+    # URI host may have ipv6 enclosed by brackets and is fine according to
+    # rfc3986. The host http header must contains the brackets, ie: the same
+    # value as returned by URI#host
+    ip_host = @host[0] == '[' && @host[-1] == ']' ? @host[1..-2] : @host


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

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

bcardiff · 2018-05-31T15:31:00Z

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 · 2018-05-31T15:33:44Z

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

RX14 · 2018-05-31T16:00:59Z

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

bcardiff · 2018-05-31T16:12:23Z

@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 · 2018-05-31T16:13:16Z

@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 · 2018-06-01T08:38:45Z

@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 · 2018-06-02T13:42:31Z

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 · 2018-06-02T17:27:30Z

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 · 2018-06-02T17:29:54Z

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 · 2018-06-04T08:53:30Z

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

bcardiff · 2018-06-04T10:55:48Z

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

Fixes crystal-lang#5514

j8r reviewed May 31, 2018

View reviewed changes

Allow HTTP::Client to use ipv6 addresses

7f478a7

Fixes crystal-lang#5514

bcardiff force-pushed the fix/uri-ipv6 branch from 66b2d74 to 7f478a7 Compare May 31, 2018 10:08

RX14 reviewed May 31, 2018

View reviewed changes

Move unwrap of brackets to URI.hostname

7ac9d1c

Use String#starts_with? and String#ends_with?

d0f13d3

Mark URI.hostname as :nodoc:

a768ccd

Remove URI.hostname, duplicate a bit of code

9430f07

RX14 approved these changes Jun 4, 2018

View reviewed changes

bcardiff added kind:bug topic:stdlib labels Jun 4, 2018

bcardiff merged commit dbd7acd into crystal-lang:master Jun 4, 2018

bcardiff added this to the 0.25.0 milestone Jun 4, 2018

bcardiff deleted the fix/uri-ipv6 branch June 4, 2018 14:15

chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018

Allow HTTP::Client to use ipv6 addresses (crystal-lang#6147)

fff4881

Fixes crystal-lang#5514

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow HTTP::Client to use ipv6 addresses #6147

Allow HTTP::Client to use ipv6 addresses #6147

bcardiff commented May 31, 2018

j8r May 31, 2018 •

edited

RX14 May 31, 2018 •

edited

RX14 May 31, 2018

j8r May 31, 2018

RX14 May 31, 2018

RX14 May 31, 2018

bcardiff commented May 31, 2018

j8r commented May 31, 2018

RX14 commented May 31, 2018 •

edited

bcardiff commented May 31, 2018

RX14 commented May 31, 2018 •

edited

bcardiff commented Jun 1, 2018

RX14 commented Jun 2, 2018

bcardiff commented Jun 2, 2018

RX14 commented Jun 2, 2018 •

edited

ysbaddaden commented Jun 4, 2018

bcardiff commented Jun 4, 2018

Allow HTTP::Client to use ipv6 addresses #6147

Allow HTTP::Client to use ipv6 addresses #6147

Conversation

bcardiff commented May 31, 2018

j8r May 31, 2018 • edited

Choose a reason for hiding this comment

RX14 May 31, 2018 • edited

Choose a reason for hiding this comment

RX14 May 31, 2018

Choose a reason for hiding this comment

j8r May 31, 2018

Choose a reason for hiding this comment

RX14 May 31, 2018

Choose a reason for hiding this comment

RX14 May 31, 2018

Choose a reason for hiding this comment

bcardiff commented May 31, 2018

j8r commented May 31, 2018

RX14 commented May 31, 2018 • edited

bcardiff commented May 31, 2018

RX14 commented May 31, 2018 • edited

bcardiff commented Jun 1, 2018

RX14 commented Jun 2, 2018

bcardiff commented Jun 2, 2018

RX14 commented Jun 2, 2018 • edited

ysbaddaden commented Jun 4, 2018

bcardiff commented Jun 4, 2018

j8r May 31, 2018 •

edited

RX14 May 31, 2018 •

edited

RX14 commented May 31, 2018 •

edited

RX14 commented May 31, 2018 •

edited

RX14 commented Jun 2, 2018 •

edited