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

Hide implementation details #261

Closed
wants to merge 2 commits into from

Conversation

kwando
Copy link
Contributor

@kwando kwando commented Oct 9, 2015

Dont leak implementation details error from the connection class.

SocketError and SystemCallError is rescued and reraised as a HTTP::Error instead.

Maybe there should be a HTTP::ConnectionError < HTTP::Error class.

@zanker
Copy link
Contributor

zanker commented Oct 9, 2015

Could you give some context on why you want to wrap some of the lower level errors? I'm not necessarily against it, but having the original error class can be helpful for various error programs. Since a ECONNREFUSED can be different from the other various ECONN type errors.

@kwando
Copy link
Contributor Author

kwando commented Oct 9, 2015

If I wanna catch all errors that http is throwing I essentially have to rescue from Exception, which is a bad for a number of reasons. Or I need to read the source code to figure out which exceptions this lib might throw, since the documentation is not telling me.

Have a typo in a domain or a host that is just not reachable is pretty well inside the core domain of a HTTP lib hence I think those exceptions should be rescued and reraised to something decending from HTTP::Error.

@tarcieri
Copy link
Member

tarcieri commented Oct 9, 2015

What error are you observing that isn't descended from StandardError?

In general rescue => ex (sans Exception) should cover all cases for the purposes of this library.

@kwando
Copy link
Contributor Author

kwando commented Oct 9, 2015

HTTP.get("http://127.0.0.1:3456")

Where there is nothing responding on 3456 on localhost

@tarcieri
Copy link
Member

tarcieri commented Oct 9, 2015

[1] pry(main)> require 'http'
=> true
[2] pry(main)> begin
[2] pry(main)*   HTTP.get("http://127.0.0.1:3456")
[2] pry(main)* rescue => ex
[2] pry(main)*   puts "Uhoh, got a #{ex.class}: #{ex}"
[2] pry(main)* end
Uhoh, got a Errno::ECONNREFUSED: Connection refused - connect(2) for "127.0.0.1" port 3456
=> nil
[3] pry(main)> Errno::ECONNREFUSED.ancestors
=> [Errno::ECONNREFUSED,
 SystemCallError,
 StandardError,
 Exception,
 Object,
 JSON::Ext::Generator::GeneratorMethods::Object,
 PP::ObjectMixin,
 Kernel,
 BasicObject]

So, no?

@kwando
Copy link
Contributor Author

kwando commented Oct 9, 2015

Oh, sorry my bad for not getting my facts straight. I believed Errno::* errors decended from Exception.

So what do you think about catching both of those errors and reraise them as a HTTP::ConnectionError?

@tarcieri
Copy link
Member

Having one class that wraps up all of the network-related errors might be nice.

@ixti
Copy link
Member

ixti commented Oct 12, 2015

I like the idea of HTTP::ConnectionError < HTTP::Error

@ixti ixti self-assigned this Nov 16, 2015
@ixti
Copy link
Member

ixti commented Dec 12, 2015

I have rebased and fixed that branch of @kwando
Will open a new PR.

@ixti ixti closed this Dec 12, 2015
@ixti ixti mentioned this pull request Dec 12, 2015
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