Merge pull request Netflix#1271 from daniilguit/master

Fix race condition in HystrixThreadPoolKey.asKey method

hystrix-core/build.gradle

@@ -5,7 +5,7 @@ dependencies {
     compile 'com.netflix.archaius:archaius-core:0.4.1'
     compile 'io.reactivex:rxjava:1.1.5'
     compile 'org.slf4j:slf4j-api:1.7.0'
-	compile 'org.hdrhistogram:HdrHistogram:2.1.7'
+    compile 'org.hdrhistogram:HdrHistogram:2.1.7'
     testCompile 'junit:junit-dep:4.10'
     testCompile project(':hystrix-junit')
 }
@@ -16,8 +16,8 @@ javadoc {
     // we do not want the com.netflix.hystrix.util package include
     exclude '**/util/**'
 
-        options {
-	doclet = "org.benjchristensen.doclet.DocletExclude"
+    options {
+        doclet = "org.benjchristensen.doclet.DocletExclude"
         docletpath = [rootProject.file('./gradle/doclet-exclude.jar')]
         stylesheetFile = rootProject.file('./gradle/javadocStyleSheet.css')
         windowTitle = "Hystrix Javadoc ${project.version}"
@@ -37,12 +37,12 @@ jar {
 }
 
 jmh {
-	fork = 10
-	iterations = 3
-	jmhVersion = '1.11.2'
-	profilers = ['gc']
-	threads = 8
-	warmup = '1s'
-	warmupBatchSize = 1
-	warmupIterations = 5
+    fork = 10
+    iterations = 3
+    jmhVersion = '1.11.2'
+    profilers = ['gc']
+    threads = 8
+    warmup = '1s'
+    warmupBatchSize = 1
+    warmupIterations = 5
 }

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

@@ -15,7 +15,7 @@
  */
 package com.netflix.hystrix;
 
-import java.util.concurrent.ConcurrentHashMap;
+import com.netflix.hystrix.util.InternMap;
 
 /**
  * A group name for a {@link HystrixCommand}. This is used for grouping together commands such as for reporting, alerting, dashboards or team/library ownership.
@@ -24,51 +24,36 @@
  * <p>
  * This interface is intended to work natively with Enums so that implementing code can have an enum with the owners that implements this interface.
  */
-public interface HystrixCommandGroupKey {
-
-    /**
-     * The word 'name' is used instead of 'key' so that Enums can implement this interface and it work natively.
-     * 
-     * @return String
-     */
-    public String name();
-
-    public static class Factory {
-
+public interface HystrixCommandGroupKey extends HystrixKey {
+    class Factory {
         private Factory() {
         }
 
         // used to intern instances so we don't keep re-creating them millions of times for the same key
-        private static ConcurrentHashMap<String, HystrixCommandGroupKey> intern = new ConcurrentHashMap<String, HystrixCommandGroupKey>();
+        private static final InternMap<String, HystrixCommandGroupDefault> intern
+                = new InternMap<String, HystrixCommandGroupDefault>(
+                new InternMap.ValueConstructor<String, HystrixCommandGroupDefault>() {
+                    @Override
+                    public HystrixCommandGroupDefault create(String key) {
+                        return new HystrixCommandGroupDefault(key);
+                    }
+                });
+
 
         /**
          * Retrieve (or create) an interned HystrixCommandGroup instance for a given name.
-         * 
+         *
          * @param name command group name
          * @return HystrixCommandGroup instance that is interned (cached) so a given name will always retrieve the same instance.
          */
         public static HystrixCommandGroupKey asKey(String name) {
-            HystrixCommandGroupKey k = intern.get(name);
-            if (k == null) {
-                k = new HystrixCommandGroupDefault(name);
-                intern.putIfAbsent(name, k);
-            }
-            return k;
+           return intern.interned(name);
         }
 
-        private static class HystrixCommandGroupDefault implements HystrixCommandGroupKey {
-
-            private String name;
-
-            private HystrixCommandGroupDefault(String name) {
-                this.name = name;
+        private static class HystrixCommandGroupDefault extends HystrixKey.HystrixKeyDefault implements HystrixCommandGroupKey {
+            public HystrixCommandGroupDefault(String name) {
+                super(name);
             }
-
-            @Override
-            public String name() {
-                return name;
-            }
-
         }
 
         /* package-private */ static int getGroupCount() {

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

@@ -15,29 +15,28 @@
  */
 package com.netflix.hystrix;
 
-import java.util.concurrent.ConcurrentHashMap;
+import com.netflix.hystrix.util.InternMap;
 
 /**
  * A key to represent a {@link HystrixCommand} for monitoring, circuit-breakers, metrics publishing, caching and other such uses.
  * <p>
  * This interface is intended to work natively with Enums so that implementing code can be an enum that implements this interface.
  */
-public interface HystrixCommandKey {
-
-    /**
-     * The word 'name' is used instead of 'key' so that Enums can implement this interface and it work natively.
-     * 
-     * @return String
-     */
-    public String name();
-
-    public static class Factory {
-
+public interface HystrixCommandKey extends HystrixKey {
+    class Factory {
         private Factory() {
         }
 
         // used to intern instances so we don't keep re-creating them millions of times for the same key
-        private static ConcurrentHashMap<String, HystrixCommandKey> intern = new ConcurrentHashMap<String, HystrixCommandKey>();
+        private static final InternMap<String, HystrixCommandKeyDefault> intern
+                = new InternMap<String, HystrixCommandKeyDefault>(
+                new InternMap.ValueConstructor<String, HystrixCommandKeyDefault>() {
+                    @Override
+                    public HystrixCommandKeyDefault create(String key) {
+                        return new HystrixCommandKeyDefault(key);
+                    }
+                });
+
 
         /**
          * Retrieve (or create) an interned HystrixCommandKey instance for a given name.
@@ -46,30 +45,12 @@ private Factory() {
          * @return HystrixCommandKey instance that is interned (cached) so a given name will always retrieve the same instance.
          */
         public static HystrixCommandKey asKey(String name) {
-            HystrixCommandKey k = intern.get(name);
-            if (k == null) {
-                k = new HystrixCommandKeyDefault(name);
-                intern.putIfAbsent(name, k);
-            }
-            return k;
+            return intern.interned(name);
         }
 
-        private static class HystrixCommandKeyDefault implements HystrixCommandKey {
-
-            private String name;
-
-            private HystrixCommandKeyDefault(String name) {
-                this.name = name;
-            }
-
-            @Override
-            public String name() {
-                return name;
-            }
-
-            @Override
-            public String toString() {
-            	return name;
+        private static class HystrixCommandKeyDefault extends HystrixKey.HystrixKeyDefault implements HystrixCommandKey {
+            public HystrixCommandKeyDefault(String name) {
+                super(name);
             }
         }
 

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

@@ -0,0 +1,34 @@
+package com.netflix.hystrix;
+
+/**
+ * Basic class for hystrix keys
+ */
+public interface HystrixKey {
+    /**
+     * The word 'name' is used instead of 'key' so that Enums can implement this interface and it work natively.
+     *
+     * @return String
+     */
+    String name();
+
+    /**
+     * Default implementation of the interface
+     */
+    abstract class HystrixKeyDefault implements HystrixKey {
+        private final String name;
+
+        public HystrixKeyDefault(String name) {
+            this.name = name;
+        }
+
+        @Override
+        public String name() {
+            return name;
+        }
+
+        @Override
+        public String toString() {
+            return name;
+        }
+    }
+}

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

@@ -15,31 +15,27 @@
  */
 package com.netflix.hystrix;
 
-import java.util.concurrent.ConcurrentHashMap;
+import com.netflix.hystrix.util.InternMap;
 
 /**
  * A key to represent a {@link HystrixThreadPool} for monitoring, metrics publishing, caching and other such uses.
  * <p>
  * This interface is intended to work natively with Enums so that implementing code can be an enum that implements this interface.
  */
-public interface HystrixThreadPoolKey {
-
-    /**
-     * The 'key' used as the name for a thread-pool.
-     * <p>
-     * The word 'name' is used instead of 'key' so that Enums can implement this interface and it work natively.
-     * 
-     * @return String
-     */
-    public String name();
-
-    public static class Factory {
-
+public interface HystrixThreadPoolKey extends HystrixKey {
+    class Factory {
         private Factory() {
         }
 
         // used to intern instances so we don't keep re-creating them millions of times for the same key
-        private static ConcurrentHashMap<String, HystrixThreadPoolKey> intern = new ConcurrentHashMap<String, HystrixThreadPoolKey>();
+        private static final InternMap<String, HystrixThreadPoolKey> intern
+                = new InternMap<String, HystrixThreadPoolKey>(
+                new InternMap.ValueConstructor<String, HystrixThreadPoolKey>() {
+                    @Override
+                    public HystrixThreadPoolKey create(String key) {
+                        return new HystrixThreadPoolKeyDefault(key);
+                    }
+                });
 
         /**
          * Retrieve (or create) an interned HystrixThreadPoolKey instance for a given name.
@@ -48,35 +44,17 @@ private Factory() {
          * @return HystrixThreadPoolKey instance that is interned (cached) so a given name will always retrieve the same instance.
          */
         public static HystrixThreadPoolKey asKey(String name) {
-            HystrixThreadPoolKey k = intern.get(name);
-            if (k == null) {
-                k = new HystrixThreadPoolKeyDefault(name);
-                intern.putIfAbsent(name, k);
-            }
-            return k;
+           return intern.interned(name);
         }
 
-        private static class HystrixThreadPoolKeyDefault implements HystrixThreadPoolKey {
-
-            private String name;
-
-            private HystrixThreadPoolKeyDefault(String name) {
-                this.name = name;
-            }
-
-            @Override
-            public String name() {
-                return name;
-            }
-
-            @Override
-            public String toString() {
-            	return name;
+        private static class HystrixThreadPoolKeyDefault extends HystrixKeyDefault implements HystrixThreadPoolKey {
+            public HystrixThreadPoolKeyDefault(String name) {
+                super(name);
             }
         }
 
         /* package-private */ static int getThreadPoolCount() {
             return intern.size();
         }
     }
-}
+}

hystrix-core/src/main/java/com/netflix/hystrix/util/InternMap.java

@@ -0,0 +1,34 @@
+package com.netflix.hystrix.util;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+/**
+ * Utility to have 'intern' - like functionality, which holds single instance of wrapper for a given key
+ */
+public class InternMap<K, V> {
+    private final ConcurrentMap<K, V> storage = new ConcurrentHashMap<K, V>();
+    private final ValueConstructor<K, V> valueConstructor;
+
+    public interface ValueConstructor<K, V> {
+        V create(K key);
+    }
+
+    public InternMap(ValueConstructor<K, V> valueConstructor) {
+        this.valueConstructor = valueConstructor;
+    }
+
+    public V interned(K key) {
+        V existingKey = storage.get(key);
+        V newKey = null;
+        if (existingKey == null) {
+            newKey = valueConstructor.create(key);
+            existingKey = storage.putIfAbsent(key, newKey);
+        }
+        return existingKey != null ? existingKey : newKey;
+    }
+
+    public int size() {
+        return storage.size();
+    }
+}