httpi icon indicating copy to clipboard operation
httpi copied to clipboard

Unified HTTP Exceptions?

Open eimermusic opened this issue 13 years ago • 11 comments

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?

eimermusic avatar Jan 10 '11 13:01 eimermusic

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

rubiii avatar Jan 10 '11 13:01 rubiii

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

vangberg avatar Jan 11 '11 15:01 vangberg

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.

eimermusic avatar Jan 11 '11 15:01 eimermusic

@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.

lgustafson avatar Jan 19 '13 01:01 lgustafson

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.

coldnebo avatar Sep 08 '13 02:09 coldnebo

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

rogerleite avatar Sep 09 '13 14:09 rogerleite

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 avatar Jan 14 '15 15:01 chetan

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?

chetan avatar Jan 14 '15 15:01 chetan

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

rogerleite avatar Jan 14 '15 15:01 rogerleite

@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.

tjarratt avatar Jan 15 '15 06:01 tjarratt

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.

907th avatar Aug 24 '18 08:08 907th