Allow Features to handle ConnectionErrors

[joshuaflanagan]

An obvious use case for the Feature extension point is for logging/instrumenting (https://github.com/httprb/http/pull/499). This allows you to log request details, and the corresponding response. However, if the connection fails (timeout, DNS lookup failure, etc), you only get a chance to log the request - never the response. Someone looking at the logs will not know what happened to the request.

I propose adding the Feature#on_error hook, so that logging/instrumenting extensions get a chance to log errors. It could be added in a backward compatible way, so that all existing Feature implementations still work.

Something like this:

    rescue ConnectionError => e
      options.features.each do |_name, feature|
        if feature.respond_to?(:on_error)
          feature.on_error(req, e)
        end
      end
      raise

The feature on_error callback gets access to the Request and the ConnectionError (or it could be a broader error).

I can put together a PR if we come to agreement on the behavior.

[ixti]

I'm not sure I understand why existing code won't work. You can log request before it has been sent. Not sure what benefits on_error will provide.

[joshuaflanagan]

Currently, you can configure an instance using the Logging feature, and wherever you use it, you will see a Request logged, and then the corresponding Response. The callsites dont have to know about the logging, it happens transparently by the Feature. Now, if you make a request that doesn't succeed because of a connection failure, you will log the request, but your logs wont show the result of that request - because there was no response. In order to log the connection failure, you will have to directly instrument at every callsite.

You would have to do something like this, for every call:

begin
  http.get("http://www.example.com")
rescue ConnectionError => e
  <same logger configured by HTTP Logging feature>.info("ConnectionError: #{e}"
end

Alternatively, you can add the on_error handler, and the Logging feature could log that error for you - without the callsite having to handle it.

[ixti]

I still don't think it's a good idea to introduce the change that will be useful only for logging feature.

[joshuaflanagan]

I got the impression that the Feature API was designed specifically for logging/instrumentation, and then made slightly more generic to allow some other middleware support. Currently, the only other Features are inflate/deflate. NormalizeUri is a strange one - I'm not sure why that is a Feature. So it seems like tying the logging/instrumentation implementation to the generic Feature API has limited its usefulness. Would you be more open to adding first-class logging support directly in the Client?

[paul]

@ixti I think the issue is that there's http-level errors that do generate a response that the Features can "see", but there's another class of errors below the http level (connection errors, timeouts, dns, etc...) that don't ever generate an http response. Since (i think) http.rb just raises an exception in that case, there's no visibility from within the features that it happened.

In rack-style middlewares, since each one calls the next in a nested block fashion, each gets an opportunity to rescue the exception and do something with it (log then re-raise, handle it and render an error page, etc...). But those style have the giant nested callstack issues, which I know @tarcieri is pretty strongly against.

It does seem useful for logging and instrumentation for the other class of errors to be visible somehow. #wrap_response doesn't seem like a good place, since there's no HTTP::Response involved. Something like what @joshuaflanagan has proposed, (maybe #wrap_connection_error?) that would get called for each Feature would solve this. It could be useful for more than just logging & instrumentation, I could envision a Feature that might automatically retry a connection timeout, or round-robin to try a different server (but I don't think the Features API supports intercepting a request like that yet).

[tarcieri]

@paul can't you implement Rack-style middleware as a feature? possibly with some modifications to the existing feature code (which would permit the general pattern it seems is desired in this PR)

I'm not completely against the concept, I just look at something like Faraday as being very slow because every single request goes through multiple middlewares by default. I think it's fine to have middleware optional, so long as it's off-by-default so the default case is still speedy.