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

Improve error message for getaddrinfo failure #8498

Merged
merged 7 commits into from
Jan 7, 2020

Conversation

rdp
Copy link
Contributor

@rdp rdp commented Nov 21, 2019

…s may be connected to DNS #8262
See discussion in #8262
I wasn't sure if the hint were appropriate or not but maybe useful to someone? :)

@straight-shoota
Copy link
Member

Runtime error messages should be concise and describe the problem only as much as necessary. This seems to extensive.
How to deal with such an error is a different thing. There may be many reasons why address resolution fails. It shouldn't be Crystals job to provide general troubleshooting advice in an error message.

The only useful improvement IMO is to add context information to all errors, not just for EAI_NONAME. When address resolution fails, it's essential to know which address was tried.

@straight-shoota
Copy link
Member

BTW I tried this in several programming languages and most simply just pass the error message from getaddrinfo with no context information.

The only good message I found was in Golang:

lookup badhostname on 192.168.1.1:53: no such host

It's great to show which DNS server was used.

@rdp
Copy link
Contributor Author

rdp commented Nov 22, 2019

Whoa, golang doing their own DNS resolution, nice.
How about this instead: "getaddrinfo: for badhostname:80 over IP when attempting domain name resolution" ?

@straight-shoota
Copy link
Member

I'd go with a very concise message: Hostname lookup for badhostname failed: #{errno_message}.

  • We can skip getaddrinfo: at the beginning. This API call is only useful when the errno message is directly passed. Here, we have a customized message anyway.
  • protocol and socket are usually not very helpful. I'd omit them unless the error is EAI_SOCKTYPE or EAI_SERVICE, respectively. Similarly, the address family could be shown for EAI_ADDRFAMILY. In all other cases, lookup failed generally and these details don't matter.

@rdp
Copy link
Contributor Author

rdp commented Dec 4, 2019

OK I didn't see EAI_ADDRFAMILY defined so didn't include that but did some for the rest, please review, thank you!

src/socket/addrinfo.cr Outdated Show resolved Hide resolved
Co-Authored-By: Sijawusz Pur Rahnama <sija@sija.pl>
src/socket/addrinfo.cr Outdated Show resolved Hide resolved
@rdp
Copy link
Contributor Author

rdp commented Dec 5, 2019

Looks like this breaks some other specs, I'll check those out.

@rdp
Copy link
Contributor Author

rdp commented Dec 6, 2019

OK ready for review, thank you.

@RX14 RX14 added this to the 0.33.0 milestone Dec 16, 2019
@straight-shoota straight-shoota changed the title attempt to give a more user friendly message that getaddrinfo failure… Improve error message for getaddrinfo failure Jan 7, 2020
@straight-shoota straight-shoota merged commit f0731b9 into crystal-lang:master Jan 7, 2020
@straight-shoota
Copy link
Member

Thank you @rdp ❤️

@rdp
Copy link
Contributor Author

rdp commented Jan 8, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants