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

Allow Features to handle ConnectionErrors #547

Open
joshuaflanagan opened this issue Apr 30, 2019 · 6 comments
Open

Allow Features to handle ConnectionErrors #547

joshuaflanagan opened this issue Apr 30, 2019 · 6 comments

Comments

@joshuaflanagan
Copy link
Contributor

An obvious use case for the Feature extension point is for logging/instrumenting (#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
Copy link
Member

ixti commented Apr 30, 2019

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
Copy link
Contributor Author

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
Copy link
Member

ixti commented May 8, 2019

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

@joshuaflanagan
Copy link
Contributor Author

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
Copy link
Contributor

paul commented May 8, 2019

@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
Copy link
Member

tarcieri commented May 8, 2019

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

joshuaflanagan added a commit to ShippingEasy/http that referenced this issue May 20, 2019
This addresses httprb#547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.
joshuaflanagan added a commit to ShippingEasy/http that referenced this issue May 20, 2019
This addresses httprb#547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.
joshuaflanagan added a commit to ShippingEasy/http that referenced this issue May 20, 2019
This addresses httprb#547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.
joshuaflanagan added a commit to ShippingEasy/http that referenced this issue May 21, 2019
This addresses httprb#547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.
joshuaflanagan added a commit to ShippingEasy/http that referenced this issue May 21, 2019
This addresses httprb#547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.
joshuaflanagan added a commit to ShippingEasy/http that referenced this issue Jul 18, 2019
This addresses httprb#547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.

> Extracted the build_response method to stay under the rubocop enforced method length limit.
joshuaflanagan added a commit to ShippingEasy/http that referenced this issue Oct 21, 2019
This addresses httprb#547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.

> Extracted the build_response method to stay under the rubocop enforced method length limit.
tarcieri pushed a commit that referenced this issue Oct 21, 2019
This addresses #547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.

> Extracted the build_response method to stay under the rubocop enforced method length limit.
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

No branches or pull requests

4 participants