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

Fix ipv6 hostname connection #184

Closed
wants to merge 2 commits into from
Closed

Fix ipv6 hostname connection #184

wants to merge 2 commits into from

Conversation

j8r
Copy link
Contributor

@j8r j8r commented Aug 9, 2019

No description provided.

@will
Copy link
Owner

will commented Aug 10, 2019

Can you please explain what the fix is? Like an example of how it was going wrong and now right? Is it testable to prevent it from breaking in the future?

@j8r
Copy link
Contributor Author

j8r commented Aug 10, 2019

It was @MrSorcus that reported the issue initially.
There was a No address found for [::]:5432 over TCP (Socket::Addrinfo::Error) when using an IPv6 address like DB.open("postgres://test@[::]:5432/test_db").
Now, it works with the patch.

@straight-shoota
Copy link
Contributor

crystal-lang/crystal#8066 fixes the same bug with HTTP::Websocket.

A spec would be great here, too!

@asterite
Copy link
Contributor

I would keep optimizations and other stylistic changes outside of this PR, though, they make it harder to review (and some. changes I don't agree, and will might not agree either)

@j8r
Copy link
Contributor Author

j8r commented Aug 10, 2019

@asterite not if you review per commit, there are very few changes so I think it's OK.
For the spec, sure I add it one right now.

@j8r
Copy link
Contributor Author

j8r commented Aug 12, 2019

Ready for review

@will
Copy link
Owner

will commented Aug 12, 2019

Thanks! I rebased and merged this, but outside of github so it looks like it can't figure it out.

@will will closed this Aug 12, 2019
@j8r j8r deleted the fix-ipv6-hostname-connection branch August 13, 2019 07:03
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.

None yet

4 participants