yammer / circuitbox

Circuit breaker built with large Ruby apps in mind.
Other
704 stars 59 forks source link

Circuitbox with moneta cache break my faraday requests when redis is with any connection error #178

Closed cfmarques closed 3 years ago

cfmarques commented 3 years ago

When we are using circuitbox with moneta cache with redis and the redis presents any connection error, the faraday requests are broken.

The error occours because the circuitbox faraday middleware: https://github.com/yammer/circuitbox/blob/fdc9ba3725d63b1552d1c8672f56cb0c98d50ed5/lib/circuitbox/faraday_middleware.rb#L60

It try use the moneta with redis (Configurated previous as circuit_store) to cache and increment notification, but when redis presents any connection error the Circuitbox lib doens't treat the error and raises error and interrupting the requests: https://github.com/yammer/circuitbox/blob/e072db6d9882d9336462aee3c58f0bb33b8ff857/lib/circuitbox/circuit_breaker.rb#L200

In your opinion:

  1. The Circuitbox lib needs prevent this comportment (Broken faraday requests when redis depedency is out) using some code like this:

    class Circuitbox
    class FaradayMiddleware < Faraday::Middleware
    ...
    def call(request_env)
      service_response = nil
      circuit(request_env).run do
        @app.call(request_env).on_complete do |env|
          service_response = Faraday::Response.new(env)
          raise RequestFailed if open_circuit?(service_response)
        end
      end
    rescue Circuitbox::Error => e
      circuit_open_value(request_env, service_response, e)
    rescue Redis::BaseConnectionError => e
      Rails.logger.warn "Circuitbox 'down' because Redis::BaseConnectionError. Message: #{e.message}"
    
      @app.call(request_env).on_complete do |env|
        service_response = Faraday::Response.new(env)
      end
    end
    ...
    end
    end
  2. Or the Circuitbox lib needs treat the connection error and improve error legibility, something like: "circuit_store is out"?
  3. Or the application that implements Circuitbox needs understand that your redis is out and disable the circuitbreaker?
matthewshafer commented 3 years ago

I've seen issues like this brought up before so I think I can answer all of your points and hopefully help with some ways to improve the situation you are running into.

I've heard about people using different circuit stores besides the in-memory one and it doesn't make sense for Circuitbox to handle all types of errors from every store. If Circuitbox did know how to handle many different stores error conditions what should happen would likely depend on the application. In the issue we're discussing here it seems like when redis is unreachable the request should always be made. That might differ from someone else's use case where they want the circuit to be open, and the request not made, if redis is unreachable. Another issue that could happen when handled at the Circuitbox faraday middleware level is the circuit store failing after the request is made. We don't want the request to be made again because that could be very problematic, especially with requests that are mutations.

One possible way to handle the issue you are seeing is to handle errors at the circuit store level. This could be a wrapper class that has the circuit_store methods Circuitbox needs but handles the errors that can come up from from the redis Moneta store that's currently being used and returns defaults that make sense for your application. I will add that I've never tried this approach because we run all of our circuits with the in-memory store.

cfmarques commented 3 years ago

Thank you so much @matthewshafer! 🙏 ❤️

Temporally we decided disable the "Circuitbox"" when the redis is out.

But, I think that the better approuch is create the wrapper as you suggest and when a storage service is out, we storage the requests in the other service storage. When the redis is up again, we move to redis.