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

Throw DNSError in getaddrinfo (hide UVError) #15879

Merged
merged 1 commit into from
May 6, 2016

Conversation

samoconnor
Copy link
Contributor

@samoconnor samoconnor commented Apr 15, 2016

function getaddrinfo(host::ASCIIString)
c = Condition()
getaddrinfo(host) do IP
notify(c,IP)
end
ip = wait(c)
isa(ip,UVError) && throw(ip)
if isa(ip,UVError)
@assert ip.code in [UV_EAI_NONAME, UV_EAI_AGAIN, UV_EAI_FAIL, UV_EAI_NODATA]
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, an assert is not appropriate here. If the code is not in this list, rethrow it so the user can see what the code actually was.

@samoconnor samoconnor changed the title Throw DNSError in getaddrinfo Throw DNSError in getaddrinfo (hide UVError) Apr 22, 2016
@samoconnor
Copy link
Contributor Author

i would go with it. i'm still undecided on whether to change it to use direct return of a nullable error type. in the meantime, providing clearer error messages i think is still a worthwhile investment.

@vtjnash any interest in merging this?

@vtjnash
Copy link
Member

vtjnash commented Apr 27, 2016

yes, just waiting to see Tony's comment addressed (remove the assert and rethrow the UVError if it wasn't one of the expected ones)

@@ -553,7 +563,7 @@ function uv_getaddrinfocb(req::Ptr{Void}, status::Cint, addrinfo::Ptr{Void})
cb = unsafe_pointer_to_objref(data)::Function
pop!(callback_dict,cb) # using pop forces an error if cb not in callback_dict
if status != 0 || addrinfo == C_NULL
cb(UVError("getaddrinfo callback",status))
cb(UVError("uv_getaddrinfodb received an unexpected status code", status))
Copy link
Contributor

Choose a reason for hiding this comment

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

uv_getaddrinfocb

@samoconnor
Copy link
Contributor Author

@vtjnash OK, done.
Based on my reading of the libuv documentation and source code (win and unix), UVError will now only be thrown if there is a bug.

eventloop(), host, C_NULL, cb, uv_jl_getaddrinfocb::Ptr{Void})
if status == UV_EINVAL
throw(ArgumentError("Invalid uv_getaddrinfo() agument"))
elseif status in [UV_ENOMEM, UV_ENOBUFS]
Copy link
Contributor

Choose a reason for hiding this comment

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

ENOBUFS isn't really an OutOfMemoryError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is in this case.

@@ -171,6 +171,7 @@ export
TypeError,
AssertionError,
UnicodeError,
DNSError,
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's going to be exported, it needs docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I've un-exported for now.

Copy link
Member

Choose a reason for hiding this comment

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

If it's thrown, then it should be exported, right?

Copy link
Contributor

@tkelman tkelman Apr 27, 2016

Choose a reason for hiding this comment

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

we don't export UVError - we could export DNSError if it's documented and we think packages might want to throw it though

Throw UVError when an unexpected code is received.
@samoconnor
Copy link
Contributor Author

squashed

@samoconnor
Copy link
Contributor Author

@vtjnash bump

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.

4 participants