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.

Sign up for GitHub

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

Allow HTTP::Client to use ipv6 addresses #6147

Merged
merged 5 commits into from
Jun 4, 2018
Merged

Allow HTTP::Client to use ipv6 addresses #6147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f478a7
Allow HTTP::Client to use ipv6 addresses
bcardiff May 31, 2018
7ac9d1c
Move unwrap of brackets to URI.hostname
bcardiff May 31, 2018
d0f13d3
Use String#starts_with? and String#ends_with?
bcardiff May 31, 2018
a768ccd
Mark URI.hostname as :nodoc:
bcardiff Jun 1, 2018
9430f07
Remove URI.hostname, duplicate a bit of code
bcardiff Jun 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions spec/std/http/client/client_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,16 @@ module HTTP
end
end

it "sends the host header ipv6 with brackets" do
server = HTTP::Server.new do |context|
context.response.print context.request.headers["Host"]
end
address = server.bind_unused_port "::1"
spawn { server.listen }

HTTP::Client.get("http://[::1]:#{address.port}/").body.should eq("[::1]:#{address.port}")
end

it "doesn't read the body if request was HEAD" do
resp_get = TestServer.open("localhost", 0, 0) do |server|
client = Client.new("localhost", server.local_address.port)
Expand Down
1 change: 1 addition & 0 deletions spec/std/uri_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ end
describe "URI" do
assert_uri("http://www.example.com", scheme: "http", host: "www.example.com")
assert_uri("http://www.example.com:81", scheme: "http", host: "www.example.com", port: 81)
assert_uri("http://[::1]:81", scheme: "http", host: "[::1]", port: 81)
assert_uri("http://www.example.com/foo", scheme: "http", host: "www.example.com", path: "/foo")
assert_uri("http://www.example.com/foo?q=1", scheme: "http", host: "www.example.com", path: "/foo", query: "q=1")
assert_uri("http://www.example.com?q=1", scheme: "http", host: "www.example.com", query: "q=1")
Expand Down
6 changes: 5 additions & 1 deletion src/http/client.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,11 @@ class HTTP::Client
socket = @socket
return socket if socket

socket = TCPSocket.new @host, @port, @dns_timeout, @connect_timeout
# 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
Copy link
Contributor

@j8r j8r May 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@RX14 RX14 May 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

socket = TCPSocket.new ip_host, @port, @dns_timeout, @connect_timeout
socket.read_timeout = @read_timeout if @read_timeout
socket.sync = false
@socket = socket
Expand Down