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

Wrap uninformative Java exceptions in ConnectionFailure #3078

Merged
merged 1 commit into from Jan 18, 2020
Merged

Conversation

rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 18, 2020

See http4s/blaze#373.

Java's UnresolvableAddressException does not carry any information about the host that failed to resolve. If a client fails to connect, we should provide context on which host we failed to connect to. We do so by introducing a wrapper exception, ConnectionFailure, in the client package.

Other clients should probably use this if they can. Fixed for blaze, as it was reported on blaze.

hamnis
hamnis approved these changes Jan 18, 2020
@tomasherman
Copy link
Contributor

@tomasherman tomasherman commented Jan 18, 2020

awesome, thanks!

@rossabaker rossabaker merged commit e3fa942 into master Jan 18, 2020
3 checks passed
@tomasherman
Copy link
Contributor

@tomasherman tomasherman commented Jan 18, 2020

is there any chance of having this backported into the old build? Or is there an ETA on next version of http4s going stable?

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Jan 18, 2020

I'm working on 0.21-0-RC1 right now. It does subtly change the behavior, so I'm a little reluctant to backport it into 0.20, though it's difficult to imagine anyone is depending on catching the Java exception. I could be talked into it, because I'm also preparing an 0.20 backport release right now.

@tomasherman
Copy link
Contributor

@tomasherman tomasherman commented Jan 18, 2020

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Jan 19, 2020

There are many Java exceptions that can be thrown here, and we can't dynamically mix a ConnectionFailure into all of them. But we could handle this one and throw a subtype that overrides the message in 0.20, and stick with the new wrapper in 0.21.

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

3 participants