Tripped CircuitBreaker Wouldn't Close Under Contention

hystrix-core/src/main/java/com/netflix/hystrix/HystrixCircuitBreaker.java

@@ -139,10 +139,10 @@ protected HystrixCircuitBreakerImpl(HystrixCommandKey key, HystrixCommandGroupKe
 
         public void markSuccess() {
             if (circuitOpen.get()) {
-                // If we have been 'open' and have a success then we want to close the circuit. This handles the 'singleTest' logic
-                circuitOpen.set(false);
                 // TODO how can we can do this without resetting the counts so we don't lose metrics of short-circuits etc?
                 metrics.resetCounter();
+                // If we have been 'open' and have a success then we want to close the circuit. This handles the 'singleTest' logic
+                circuitOpen.set(false);
             }
         }
 
@@ -202,8 +202,10 @@ public boolean isOpen() {
                     // How could previousValue be true? If another thread was going through this code at the same time a race-condition could have
                     // caused another thread to set it to true already even though we were in the process of doing the same
                     circuitOpenedOrLastTestedTime.set(System.currentTimeMillis());
+                    return true;
+                } else {
+                    return false;
                 }
-                return true;
             }
         }
 

hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommandMetrics.java

@@ -232,8 +232,10 @@ public int getTotalTimeMean() {
     }
 
     /* package */void resetCounter() {
-        counter.reset();
         // TODO can we do without this somehow?
+        counter.reset();
+        lastHealthCountsSnapshot.set(System.currentTimeMillis());
+        healthCountsSnapshot = new HealthCounts(0, 0, 0);
     }
 
     /**

hystrix-core/src/test/java/com/netflix/hystrix/HystrixCircuitBreakerTest.java

@@ -1,11 +1,18 @@
 package com.netflix.hystrix;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
+import java.util.Random;
+
+import org.junit.Ignore;
 import org.junit.Test;
 
 import com.netflix.hystrix.HystrixCircuitBreaker.HystrixCircuitBreakerImpl;
+import com.netflix.hystrix.strategy.HystrixPlugins;
 import com.netflix.hystrix.strategy.eventnotifier.HystrixEventNotifierDefault;
+import com.netflix.hystrix.strategy.executionhook.HystrixCommandExecutionHook;
 import com.netflix.hystrix.util.HystrixRollingNumberEvent;
 
 public class HystrixCircuitBreakerTest {
@@ -494,4 +501,98 @@ private static enum CommandKeyForUnitTest implements HystrixCommandKey {
         KEY_ONE, KEY_TWO;
     }
 
+    // ignoring since this never ends ... useful for testing https://github.com/Netflix/Hystrix/issues/236
+    @Ignore
+    @Test
+    public void testSuccessClosesCircuitWhenBusy() throws InterruptedException {
+        HystrixPlugins.getInstance().registerCommandExecutionHook(new MyHystrixCommandExecutionHook());
+        try {
+            performLoad(200, 0, 40);
+            performLoad(250, 100, 40);
+            performLoad(600, 0, 40);
+        } finally {
+            Hystrix.reset();
+        }
+
+    }
+
+    void performLoad(int totalNumCalls, int errPerc, int waitMillis) {
+
+        Random rnd = new Random();
+
+        for (int i = 0; i < totalNumCalls; i++) {
+            //System.out.println(i);
+
+            try {
+                boolean err = rnd.nextFloat() * 100 < errPerc;
+
+                TestCommand cmd = new TestCommand(err);
+                cmd.execute();
+
+            } catch (Exception e) {
+                //System.err.println(e.getMessage());
+            }
+
+            try {
+                Thread.sleep(waitMillis);
+            } catch (InterruptedException e) {
+            }
+        }
+    }
+
+    public class TestCommand extends HystrixCommand<String> {
+
+        boolean error;
+
+        public TestCommand(final boolean error) {
+            super(HystrixCommandGroupKey.Factory.asKey("group"));
+
+            this.error = error;
+        }
+
+        @Override
+        protected String run() throws Exception {
+
+            if (error) {
+                throw new Exception("forced failure");
+            } else {
+                return "success";
+            }
+        }
+
+        @Override
+        protected String getFallback() {
+            if (isFailedExecution()) {
+                return getFailedExecutionException().getMessage();
+            } else {
+                return "other fail reason";
+            }
+        }
+
+    }
+
+    public class MyHystrixCommandExecutionHook extends HystrixCommandExecutionHook {
+
+        @Override
+        public <T> T onComplete(final HystrixCommand<T> command, final T response) {
+
+            logHC(command, response);
+
+            return super.onComplete(command, response);
+        }
+
+        private int counter = 0;
+
+        private <T> void logHC(HystrixCommand<T> command, T response) {
+
+            //if ((counter++ % 20) == 0) {
+            HystrixCommandMetrics metrics = command.getMetrics();
+            System.out.println("cb/error-count/%/total: "
+                    + command.isCircuitBreakerOpen() + " "
+                    + metrics.getHealthCounts().getErrorCount() + " "
+                    + metrics.getHealthCounts().getErrorPercentage() + " "
+                    + metrics.getHealthCounts().getTotalRequests() + "  => " + response + "  " + command.getExecutionEvents());
+            //}
+        }
+    }
 }