CircuitBreaker broken for 0.9.3

[sunng87]

After updated to 0.9.3, some of my tests failed:

FAIL in (test-circuit-breaker) (core_test.clj:185)
circuit open
expected: (instance? CircuitBreakerOpenException e)
  actual: java.lang.NullPointerException

lein test :only diehard.core-test/test-circuit-breaker

FAIL in (test-circuit-breaker) (core_test.clj:185)
circuit open
expected: (instance? CircuitBreakerOpenException e)
  actual: java.lang.IllegalStateException

lein test :only diehard.core-test/test-circuit-breaker

FAIL in (test-circuit-breaker) (core_test.clj:185)
circuit open
expected: (instance? CircuitBreakerOpenException e)
  actual: java.lang.IllegalStateException

Those situation expected to throw CircuitBreakerOpenException now doesn't work as expected.

I assume there shouldn't be API break change in a patch release. So this might be some regression issue?

[jhalterman]

So this is the line that's failing?

https://github.com/sunng87/diehard/blob/master/test/diehard/core_test.clj#L185

One thing that comes to mind is that the delay might be too short for this test:

https://github.com/sunng87/diehard/blob/master/test/diehard/core_test.clj#L177

Since the circuit breaker will only be in the closed state for 10 milliseconds, the breaker might transition back to closed before the test finishes, causing the test failure. Try increasing the delay to 1 minute or something (a long delay shouldn't matter in this case) which will make sure the breaker stays in the open state for the duration of the test.

[sunng87]

Hi @jhalterman , I set delay to 100 seconds and the test still fails: https://travis-ci.org/sunng87/diehard/builds/158639725

With 0.9.2 it passes with 10ms delay.

[jhalterman]

Ah, now a NullPointer :) Any chance you can print a stack trace for the NullPointerException?

[sunng87]

Got the reason. From 0.9.3, now CircuitBreakerOpenException extends FailSafeException which is a little break change:

https://github.com/jhalterman/failsafe/blob/master/src/main/java/net/jodah/failsafe/CircuitBreakerOpenException.java

I will update diehard to adapt this change.

[jhalterman]

@sunng87 Thanks. I'll add a note on this change to the changelog.

The idea is that having CircuitBreakerOpenException extend FailsafeException makes the user experience a little better, so users can just catch FailsafeException if they want. I hadn't expected any breakage from this, but good to know.