Skip to content

CircuitBreaker.allowRequest change state of isOpen #1541

Closed
@narusas

Description

@narusas

Hi.

Recently, I need to know is circuit breaker is half-opened? can I execute command?

  @Test
  public void can_read_state_of_HALF_OPEN() {
    String commandKey = "test444"; 
    executeBeforeCircuiteOpenLimit(commandKey); // 
    {
      waitHealthUpdate();
      CommandHelloWorld cmd = new CommandHelloWorld(commandKey, true); <- when second arg is true, throw execption
      assertFalse(cmd.isCircuitBreakerOpen());
      try {
        // over 50% error!!
        cmd.execute();
        fail("Must throw exception");
      } catch (HystrixRuntimeException ex) {
        // ok, circuit is opened! 
      }
      waitHealthUpdate();
      assertTrue("Error ratio is over 50%, circuit must be opened", cmd.isCircuitBreakerOpen());
      {
        CommandHelloWorld cmdAllowRequest = new CommandHelloWorld(commandKey, true);
        assertFalse("When circuit is opened, not allow request", cmdAllowRequest.allowRequest());
      }
    }

    {
      waitSleepTime();
      waitHealthUpdate();
      CommandHelloWorld cmd = new CommandHelloWorld(commandKey, false);

      assertTrue("Over sleep time, but no request tried, no status change. So circuit status is opened, too", cmd.isCircuitBreakerOpen());
      /* CHECK ALLOW */ assertTrue("But over sleep time, circuit is half opened, must allow request ", cmd.allowRequest());

      
    }
    
    { 
      CommandHelloWorld cmd = new CommandHelloWorld(commandKey, false); <- when second arg is false, not throw execption, success
     /* HERE */  cmd.execute();

      assertFalse("over sleep, first request is succcees, circut status must be  changed", cmd.isCircuitBreakerOpen());
    }
    {
      CommandHelloWorld cmd = new CommandHelloWorld(commandKey, false);
      cmd.execute();
    }

  }

  class CommandHelloWorld extends HystrixCommand<String> {
    ...
    public boolean allowRequest() {
      return circuitBreaker.allowRequest();
    }
    ...
    @Override
    protected String run() {
      if (error) {
        throw new IllegalArgumentException("No");
      }
      return "Hello " + name + "!";
    }

  }

But this test is failed at /* HERE */ .
Result: Circuit is shorted

So I remove /* CHECK ALLOW */ line (remove call allowRequest ),
Test is success.

I trace into circuitBreaker.allowRequest(),
And found ​HystrixCircuitBreakerImpl allowSingleTest method

  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;
  }

At Seconds If statement, It change circuitOpenedOrLastTestedTime.

I think this is bug. I just call query method, but it change status.

Activity

mattrjacobs

mattrjacobs commented on Apr 19, 2017

@mattrjacobs
Contributor

Thanks for the report @narusas . Tracing through the code, I think you're correct. Given that the HystrixCircuitBreaker.allowRequest is public, it should be possible to call that method without changing state. I'll work on a fix for this.

andredasilvapinto

andredasilvapinto commented on May 1, 2017

@andredasilvapinto

+1.
I would also like to have the possibility to know if the CB is currently half-open without changing its internal state.

mattrjacobs

mattrjacobs commented on May 8, 2017

@mattrjacobs
Contributor

#1568 addresses this issue. Can someone take a look to sanity-check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @mattrjacobs@narusas@andredasilvapinto

        Issue actions

          CircuitBreaker.allowRequest change state of isOpen · Issue #1541 · Netflix/Hystrix