Merge pull request #5 from loveOSS/fix-invalid-halfopen-transition

Fixed half open state transition
loveOSS · May 13, 2019 · 360bc81 · 360bc81
2 parents f08563b + 8037e4c
commit 360bc81
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 77 deletions.
diff --git a/src/PartialCircuitBreaker.php b/src/PartialCircuitBreaker.php
@@ -85,17 +85,19 @@ public function isClosed(): bool
      * @param string $state the Place state
      * @param string $service the service URI
      *
-     * @return bool
+     * @return Transaction
      */
-    protected function moveStateTo($state, $service): bool
+    protected function moveStateTo($state, $service): Transaction
     {
         $this->currentPlace = $this->places[$state];
         $transaction = SimpleTransaction::createFromPlace(
             $this->currentPlace,
             $service
         );
 
-        return $this->storage->saveTransaction($service, $transaction);
+        $this->storage->saveTransaction($service, $transaction);
+
+        return $transaction;
     }
 
     /**

diff --git a/src/SimpleCircuitBreaker.php b/src/SimpleCircuitBreaker.php
@@ -24,15 +24,15 @@ public function call(string $service, callable $fallback, array $serviceParamete
     {
         $transaction = $this->initTransaction($service);
 
-        try {
-            if ($this->isOpened()) {
-                if ($this->canAccessService($transaction)) {
-                    $this->moveStateTo(States::HALF_OPEN_STATE, $service);
-                }
-
+        if ($this->isOpened()) {
+            if (!$this->canAccessService($transaction)) {
                 return (string) $fallback();
             }
 
+            $transaction = $this->moveStateTo(States::HALF_OPEN_STATE, $service);
+        }
+
+        try {
             $response = $this->request($service);
             $this->moveStateTo(States::CLOSED_STATE, $service);
 

diff --git a/src/SymfonyCircuitBreaker.php b/src/SymfonyCircuitBreaker.php
@@ -38,20 +38,20 @@ public function call(string $service, callable $fallback, array $serviceParamete
     {
         $transaction = $this->initTransaction($service);
 
-        try {
-            if ($this->isOpened()) {
-                if ($this->canAccessService($transaction)) {
-                    $this->moveStateTo(States::HALF_OPEN_STATE, $service);
-                    $this->dispatch(
-                        Transitions::CHECKING_AVAILABILITY_TRANSITION,
-                        $service,
-                        $serviceParameters
-                    );
-                }
-
+        if ($this->isOpened()) {
+            if (!$this->canAccessService($transaction)) {
                 return (string) $fallback();
             }
 
+            $transaction = $this->moveStateTo(States::HALF_OPEN_STATE, $service);
+            $this->dispatch(
+                Transitions::CHECKING_AVAILABILITY_TRANSITION,
+                $service,
+                $serviceParameters
+            );
+        }
+
+        try {
             $response = $this->request($service, $serviceParameters);
             $this->moveStateTo(States::CLOSED_STATE, $service);
             $this->dispatch(

diff --git a/tests/CircuitBreakerTestCase.php b/tests/CircuitBreakerTestCase.php
@@ -45,7 +45,7 @@ protected function getSystem(): MainSystem
         return MainSystem::createFromArray(
             [
                 'open' => [0, 0.0, 1.0], // threshold 1.0s
-                'half_open' => [0, 0.2, 0.0], // timeout 0.2s to test the service
+                'half_open' => [1, 0.4, 0.0], // timeout 0.4s to test the service with a better timeout
                 'closed' => [2, 0.2, 0.0], // 2 failures allowed, 0.2s timeout
             ]
         );

diff --git a/tests/CircuitBreakerWorkflowTest.php b/tests/CircuitBreakerWorkflowTest.php
@@ -13,6 +13,11 @@
 
 class CircuitBreakerWorkflowTest extends CircuitBreakerTestCase
 {
+    /**
+     * @var int the number of seconds to wait before try to reach again the service
+     */
+    private const OPEN_THRESHOLD = 1;
+
     /**
      * When we use the circuit breaker on unreachable service
      * the fallback response is used.
@@ -22,7 +27,7 @@ class CircuitBreakerWorkflowTest extends CircuitBreakerTestCase
      */
     public function testCircuitBreakerIsInClosedStateAtStart(CircuitBreaker $circuitBreaker): void
     {
-        $this->assertSame('CLOSED', $circuitBreaker->getState());
+        $this->assertTrue($circuitBreaker->isClosed());
 
         $this->assertSame(
             '{}',
@@ -41,49 +46,22 @@ public function testCircuitBreakerIsInClosedStateAtStart(CircuitBreaker $circuit
      * @depends testCircuitBreakerIsInClosedStateAtStart
      * @dataProvider getCircuitBreakers
      */
-    public function testCircuitBreakerWillBeOpenedInCaseOfFailures(CircuitBreaker $circuitBreaker): void
+    public function testCircuitBreakerWillBeOpenInCaseOfFailures(CircuitBreaker $circuitBreaker): void
     {
         // CLOSED
-        $circuitBreaker->call('https://httpbin.org/get/foo', $this->createFallbackResponse());
-
-        $this->assertSame('OPEN', $circuitBreaker->getState());
-        $this->assertSame(
-            '{}',
-            $circuitBreaker->call(
-                'https://httpbin.org/get/foo',
-                $this->createFallbackResponse()
-            )
-        );
-    }
-
-    /**
-     * Once the threshold is reached, the circuit breaker
-     * try again to reach the service. This time, the service
-     * is not reachable.
-     *
-     * @param CircuitBreaker $circuitBreaker
-     * @depends testCircuitBreakerIsInClosedStateAtStart
-     * @depends testCircuitBreakerWillBeOpenedInCaseOfFailures
-     * @dataProvider getCircuitBreakers
-     */
-    public function testAfterTheThresholdTheCircuitBreakerMovesInHalfOpenState(CircuitBreaker $circuitBreaker): void
-    {
-        // CLOSED
-        $circuitBreaker->call('https://httpbin.org/get/foo', $this->createFallbackResponse());
-        // OPEN
-        $circuitBreaker->call('https://httpbin.org/get/foo', $this->createFallbackResponse());
+        $this->assertTrue($circuitBreaker->isClosed());
+        $response = $circuitBreaker->call('https://httpbin.org/get/foo', $this->createFallbackResponse());
+        $this->assertSame('{}', $response);
 
-        $this->waitFor(1);
-        // NOW HALF OPEN
+        //After two failed calls switch to OPEN state
+        $this->assertTrue($circuitBreaker->isOpened());
         $this->assertSame(
             '{}',
             $circuitBreaker->call(
                 'https://httpbin.org/get/foo',
                 $this->createFallbackResponse()
             )
         );
-        $this->assertSame('HALF OPEN', $circuitBreaker->getState());
-        $this->assertTrue($circuitBreaker->isHalfOpened());
     }
 
     /**
@@ -92,41 +70,31 @@ public function testAfterTheThresholdTheCircuitBreakerMovesInHalfOpenState(Circu
      *
      * @param CircuitBreaker $circuitBreaker
      * @depends testCircuitBreakerIsInClosedStateAtStart
-     * @depends testCircuitBreakerWillBeOpenedInCaseOfFailures
-     * @depends testAfterTheThresholdTheCircuitBreakerMovesInHalfOpenState
+     * @depends testCircuitBreakerWillBeOpenInCaseOfFailures
      * @dataProvider getCircuitBreakers
      */
     public function testOnceInHalfOpenModeServiceIsFinallyReachable(CircuitBreaker $circuitBreaker): void
     {
-        // CLOSED
-        $circuitBreaker->call('https://httpbin.org/get/foo', $this->createFallbackResponse());
-        // OPEN
-        $circuitBreaker->call('https://httpbin.org/get/foo', $this->createFallbackResponse());
-
-        $this->waitFor(1);
-        // NOW HALF OPEN
-        $this->assertSame(
-            '{}',
-            $circuitBreaker->call(
-                'https://httpbin.org/get/foo',
-                $this->createFallbackResponse()
-            )
-        );
-        $this->assertSame('HALF OPEN', $circuitBreaker->getState());
-        $this->assertTrue($circuitBreaker->isHalfOpened());
+        // CLOSED - first call fails (twice)
+        $this->assertTrue($circuitBreaker->isClosed());
+        $response = $circuitBreaker->call('https://httpbin.org/get/foo', $this->createFallbackResponse());
+        $this->assertSame('{}', $response);
+        $this->assertTrue($circuitBreaker->isOpened());
 
-        $this->waitFor(1);
+        // OPEN - no call to client
+        $response = $circuitBreaker->call('https://httpbin.org/get/foo', $this->createFallbackResponse());
+        $this->assertSame('{}', $response);
+        $this->assertTrue($circuitBreaker->isOpened());
+        $this->waitFor(2 * self::OPEN_THRESHOLD);
 
-        // CLOSED
+        // SWITCH TO HALF OPEN - retry to call the service
         $this->assertSame(
             '{"hello": "world"}',
             $circuitBreaker->call(
                 'https://httpbin.org/get/foo',
                 $this->createFallbackResponse()
             )
         );
-
-        $this->assertSame('CLOSED', $circuitBreaker->getState());
         $this->assertTrue($circuitBreaker->isClosed());
     }