search

danielfm / pybreaker

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

Allow concurrent execution of wrapped method #40

Closed jshiell closed 5 years ago

jshiell commented 5 years ago

Please note that I don't except this to be accepted, as it changes the concurrency guarantees around the execution of the wrapped method substantially, which could break all sorts of things for current users. But I did think it might be of interest to people using this library under workloads similar to our own, and hence thought I should suggest the change at the least.

This PR removes the locking around the wrapped method call, instead making the CircuitMemoryStorage thread-safe, and added another lock to ensure that the counter changes and failure/success behaviour are atomic. This leads to two major consequences - firstly, there's now no expectation of exclusive execution in the wrapped method; and secondly, listener calls may likewise now be concurrent.

Why this madness, you may ask? Our use case is wrapping IO heavy tasks (proxying HTTP calls for assets, in particular) and hence the coarse-but-safe locking behaviour around the wrapped method leads to a major performance impact on our servers. In this case we're very happy to allow looser guarantees to substantially increase throughput.

Finally, thanks very much for all of your work on this library - it's made our lives a lot easier, and is much appreciated!

jshiell commented 5 years ago

And, as the (single) build failure shows, I've obviously missed an edge case 😂 Ah, I love concurrency.

Edit: looks like I missed the fallback state in the CircuitRedisStorage, which is just embarrassing. I'll withdraw this for the moment in any case and give it a lot more thrashing.