Comcast / jrugged

A Java libary of robustness design patterns
Apache License 2.0
265 stars 93 forks source link

Making a call in HALF_CLOSED state #39

Closed squarejaw closed 8 years ago

squarejaw commented 9 years ago

Given the following test code:

import org.fishwife.jrugged.CircuitBreaker;
import org.fishwife.jrugged.CircuitBreakerException;

import java.util.concurrent.Callable;

public class Main {

    private static Boolean fail = true;

    public static void main(String[] args) {
        CircuitBreaker cb = new CircuitBreaker();
        for (int i = 0; i < 5; i++) {
            try {
                System.out.println(cb.getStatus());
                cb.invoke(new Callable<Object>() {
                    public Object call() throws Exception {
                        System.out.println("Made it to call()");
                        if (fail) {
                            fail = false;
                            throw new Exception();
                        }
                        System.out.println("call() passed");
                        return null;
                    };
                });
            } catch(CircuitBreakerException e) {
                System.out.println("CircuitBreakerException");
                try {
                    Thread.sleep(10000);
                } catch(InterruptedException ie) {}
            } catch (Exception e) {
                System.out.println("Exception");
            }
        }
    }
}

I get this result in the current version (3.2.2)

UP
Made it to call()
Exception
DOWN
CircuitBreakerException
DOWN
CircuitBreakerException
DEGRADED
CircuitBreakerException
DEGRADED
CircuitBreakerException

But I get this in the older version (3.1.1):

UP
Made it to call()
Exception
DOWN
CircuitBreakerException
DOWN
CircuitBreakerException
DEGRADED
Made it to call()
call() passed
UP
Made it to call()
call() passed

I was under the impression that the HALF_CLOSED (or DEGRADED) status would allow a single call to go through, but it appears to always be throwing a CircuitBreakerException before the call is made. Is there something I'm missing in order to get a CircuitBreaker in a HALF_CLOSED state to re-attempt a call?

joercampbell commented 9 years ago

Yup that seems wrong - I will see if I can take a look into this weekend. Thanks for the test code as well.

joercampbell commented 9 years ago

It looks like there is possibly a mistake in the logic between < 3.2.2 and the 3.2.2 version, I am trying to track it down. What it really looks like is an errant-ly written unit test that is attempting to validate something that may not make any sense (but somehow made sense at the time I wrote it). I am going to continue to look at it to make sure I didn't mean to do something else (swap the memory back in while I was writing it).

Fixing the case you outline causes this test - testHalfClosedWithLiveAttemptThrowsCBException to fail.

Is this causing you serious problems right now? I will try to dig to the bottom as fast as I can.

squarejaw commented 9 years ago

I'm not personally using the library in any major capacity, I've just been doing a survey of the landscape of available circuit breaker/resiliency libraries and testing a few out when I noticed this issue. In any case, I can always work with version 3.1.1 if necessary. Thanks for looking into this!

joercampbell commented 9 years ago

I pushed a fix for this into the code a little while ago - lemme know if this gets you back to what you expect. I couldn't find out why beyond me being totally silly that the thing in there made sense. A HALF CLOSED breaker should RE OPEN if error, should close on Success, which I believe that the unit tests now also prove.

squarejaw commented 9 years ago

I pulled down the project, built the jars, and reran the test code. Looks like it's working as expected now. Thanks for looking into this! :+1: