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

Problem in Circuit breaker when circuitBreakerSleepWindowInMilliseconds is too low #1550

Open
mishraawake opened this issue Apr 26, 2017 · 1 comment
Labels

Comments

@mishraawake
Copy link

mishraawake commented Apr 26, 2017

For unit test, we deliberately set circuitBreakerSleepWindowInMilliseconds as 1 mili sec. Occasionally it happens that due to high failure rate, isOpen returns true but immediately after that method allowSingleTest (code is attached) return true as well because first if statement evaluated as true. Now because of this, allowRequest returns true. And it keeps on returning true for all subsequent request because in all the cases first if statement of code block evaluated as true.

I think this is a bug because for the call to which circuit trips for the first time, allowSingleTest should not be called.

 public boolean allowSingleTest() {
            long timeCircuitOpenedOrWasLastTested = circuitOpenedOrLastTestedTime.get();
            // 1) if the circuit is open
            // 2) and it's been longer than 'sleepWindow' since we opened the circuit
            if (circuitOpen.get() && System.currentTimeMillis() > timeCircuitOpenedOrWasLastTested + properties.circuitBreakerSleepWindowInMilliseconds().get()) {
                // We push the 'circuitOpenedTime' ahead by 'sleepWindow' since we have allowed one request to try.
                // If it succeeds the circuit will be closed, otherwise another singleTest will be allowed at the end of the 'sleepWindow'.
                if (circuitOpenedOrLastTestedTime.compareAndSet(timeCircuitOpenedOrWasLastTested, System.currentTimeMillis())) {
                    // if this returns true that means we set the time so we'll return true to allow the singleTest
                    // if it returned false it means another thread raced us and allowed the singleTest before we did
                    return true;
                }
            }
            return false;
        }
@mattrjacobs
Copy link
Contributor

An unrelated issue was raised around allowRequest() being non-idempotent (#1541). I just merged in a change for that (#1568). Can you take a look at that PR to see if it will address your issue as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants