danielfm / pybreaker

Python implementation of the Circuit Breaker pattern.
BSD 3-Clause "New" or "Revised" License
528 stars 73 forks source link

Should listener.failure be called when the circuit is closed and a call fails? #16

Closed sj175 closed 7 years ago

sj175 commented 7 years ago

When the circuit breaker's state is closed and the call to the wrapped function fails the listener.failure function is not called because an exception is thrown in CircuitClosedState.on_failure. Is this behaviour intentional?

The block below is from the CircuitBreakerState class:

    def _handle_error(self, exc):
        """
        Handles a failed call to the guarded operation.
        """
        if self._breaker.is_system_error(exc):
            self._breaker._inc_counter()
            self.on_failure(exc)
            for listener in self._breaker.listeners:
                listener.failure(self._breaker, exc)
        else:
            self._handle_success()
        raise exc
danielfm commented 7 years ago

Good catch.

AFAIK this behavior is not intentional, seems more like a bug to me.

danielfm commented 7 years ago

Just pushed a new version, that hopefully fixes this issue, can you take some time to test it?

sj175 commented 7 years ago

Sure, all my tests pass at the moment but it's still fairly early in my development process.

sj175 commented 7 years ago

Actually, it looks like Redis is now a dependency, it wasn't before. If that was intentional, that's fine. Otherwise, my tests are now broken.

danielfm commented 7 years ago

Redis is an optional dependency, in case you enable the Redis backing. You should be able to use it without it though.

sj175 commented 7 years ago

I get ImportError: CircuitRedisStorage can only be used if the required dependencies exist as soon as I import pybreaker, using Python 3.6.

danielfm commented 7 years ago

This is a bug; redis should not be required to use pybreaker.

I'll see what I can do. /cc @phillbaker

phillbaker commented 7 years ago

Ouch. Sorry about that. I'll take a look tonight.

danielfm commented 7 years ago

@sj175 can you please try 0.3.3 to see if the last PR fixes the issue? Feel free to reopen this in case it didn't.

sj175 commented 7 years ago

That's fixed. Brilliant, thank you!