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

Unified HTTP Exceptions? #16

Closed
eimermusic opened this issue Jan 10, 2011 · 12 comments
Closed

Unified HTTP Exceptions? #16

eimermusic opened this issue Jan 10, 2011 · 12 comments
Labels

Comments

@eimermusic
Copy link

I would love to have httpi catch (rescue) and re-throw all adapter exceptions so that client code (e.g. my Savon SOAP client) could be a bit cleaner in its exception handling.

A connection timeout, a connection refused, a internal server error... they are all different between the libraries and also not always in a "uniform" structure within each adapter.

In a SOAP client I may want to distinguish all http related errors from SOAP errors and respond to them in a certain way as a group. I may want to try a backup endpoint, for example. Something I would not want to do for SOAP exceptions but which might be a good idea for timeouts and server errors.

I haven't looked at all adapters in detail to inventory the possible exceptions in full. Excon had them all nicely bundled together... about 45-48 of them it looks like. https://github.com/geemus/excon/blob/master/lib/excon/errors.rb

What do you think? Good idea? Bad idea? A lot of work?

@rubiii
Copy link
Contributor

rubiii commented Jan 10, 2011

good idea and (possibly) a lot of work :)

@vangberg
Copy link
Contributor

For stuff like Timeout etc., this should definitely be done. But for HTTP errors, we already got Response#error? and Response#code.

@eimermusic
Copy link
Author

The thing is that at least some of the http libs throw exceptions on "bad" response codes like "not acceptable" or "forbidden".

I am not sure they can all be supressed. Using httpi with Savon and setting " Savon.raise_errors = false" I still get exceptions for network activity.

The point if to be able to catch all relevant exceptions in a clean manner. Catching all exceptions that are subclasses of, say, HTTPI::NetworkError or some such would be very nice.

I am playing with implementing an Excon adapter... Maybe that will imprive my insight into httpi a bit.

@lgustafson
Copy link
Contributor

@eimermusic, I believe Net::HTTP will raise certain exceptions under Exception rather than StandardError when they are network related. This means that they will not be rescued by the typical rescue => e. Rather, you need to rescue Exception => e.

I agree with you this would be a good feature, and I would think the nested_exceptions gem could be used to capture the underlying exception.

@coldnebo
Copy link
Contributor

coldnebo commented Sep 8, 2013

I could see a way this could be done more easily.

In Savon, wrap use of httpi with

begin
  httpi.call
rescue Exception => e
  raise SavonError(e)
end

As @lgustafson points out, exception chaining is needed, but this would allow all underlying connection and transmission errors to be caught and rethrown as one unified error. Callers could always inspect the chained error to discover the underlying cause.

I've done something similar in a work project by wrapping my class' method_missing delegate call to a Savon model underneath. Everything that can go wrong there is a kind of ServiceError.

BTW, chaining is easy if you own the exception class, no need to use the gem if you don't want the fancy backtrace (which may not be as useful to external Savon users vs. Savon maintainers):

class SavonError < RuntimeError
  def initialize(msg=nil, cause=nil)
    @cause = cause
    super(msg)
  end

  def to_s
    super.to_s + ": caused by " +  @cause.to_s
  end
end

or something similar.

Just some ideas.

@rogerleite
Copy link
Member

Your great idea give me a nice idea! Thanks @coldnebo ... if i found some time after lunch, i'll start something crazy.

@chetan
Copy link
Contributor

chetan commented Jan 14, 2015

Looks like this ticket can actually be closed but I was wondering why HTTPI::ConnectionError doesn't extend from HTTPI::Error like all the others do? Seems like an oversight?

https://github.com/savonrb/httpi/blob/master/lib/httpi.rb#L91

@chetan
Copy link
Contributor

chetan commented Jan 14, 2015

Ah, just realized it's a module and not a class, in order to support this usage:

https://github.com/savonrb/httpi/blob/master/lib/httpi/adapter/net_http.rb#L47-L49

Though I don't really understand the reason for this either. Why not just raise ConnectionError in the same way as the others?

@rogerleite
Copy link
Member

Hi @chetan.
This is really old and i don't know why this ConnectionError is a module. Maybe is something related to savon. 😕

@tjarratt
Copy link
Contributor

@chetan -- glad you brought this up.

Seems like the behavior to handle ConnectionError in this way came into the codebase in December 2012. I don't think there's a strong argument to be made for doing it this way -- raising ConnectionError like you suggested seems a lot better to me. Consistency is better than clever code, in my humble opinion.

@907th
Copy link

907th commented Aug 24, 2018

The reason for extending last error $! with a module (e.g. ConnectionError) is that exception can be catched with a module now:

module ErrTrait; end
class MyErr < StandardError; end

begin
  b = MyErr.new("msg")
  b.extend ErrTrait
  fail b
rescue ErrTrait => e
  puts e.inspect # #<MyErr: msg>
end

e.is_a?(ErrTrait) returns true now, and ErrTrait acts like a common 'tag' for different kinds of exceptions.

Another solution is: reraise errors from rescue block using different error class. But in this case it is harder to detect original error which caused this error to happen (Exception#cause is available since Ruby 2.1 to do that). It is also a breaking change, because library users may already rely on catching some specific error classes.

@pcai
Copy link
Member

pcai commented Jul 6, 2024

I consider this closed unless someone explains how to proceed - please see #238. thanks

@pcai pcai closed this as completed Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

10 participants