diff options
| -rw-r--r-- | lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java | 28 | ||||
| -rw-r--r-- | lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java | 38 |
2 files changed, 54 insertions, 12 deletions
diff --git a/lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java b/lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java index 445f7e474..753439045 100644 --- a/lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java +++ b/lib/java/src/org/apache/thrift/meta_data/FieldMetaData.java @@ -21,6 +21,8 @@ package org.apache.thrift.meta_data; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + import org.apache.thrift.TBase; import org.apache.thrift.TFieldIdEnum; @@ -28,15 +30,22 @@ import org.apache.thrift.TFieldIdEnum; * This class is used to store meta data about thrift fields. Every field in a * a struct should have a corresponding instance of this class describing it. * + * The meta data is registered by ALL Thrift struct classes via a static {...} + * initializer block in the generated Thrift code. + * + * Since different threads could be initializing different Thrift classes, calls + * to the public static methods of this class could be racy. + * + * All methods of this class should be made thread safe. */ public class FieldMetaData implements java.io.Serializable { public final String fieldName; public final byte requirementType; public final FieldValueMetaData valueMetaData; - private static Map<Class<? extends TBase>, Map<? extends TFieldIdEnum, FieldMetaData>> structMap; + private static final Map<Class<? extends TBase>, Map<? extends TFieldIdEnum, FieldMetaData>> structMap; static { - structMap = new HashMap<Class<? extends TBase>, Map<? extends TFieldIdEnum, FieldMetaData>>(); + structMap = new ConcurrentHashMap<Class<? extends TBase>, Map<? extends TFieldIdEnum, FieldMetaData>>(); } public FieldMetaData(String name, byte req, FieldValueMetaData vMetaData){ @@ -45,7 +54,7 @@ public class FieldMetaData implements java.io.Serializable { this.valueMetaData = vMetaData; } - public static synchronized void addStructMetaDataMap(Class<? extends TBase> sClass, Map<? extends TFieldIdEnum, FieldMetaData> map){ + public static void addStructMetaDataMap(Class<? extends TBase> sClass, Map<? extends TFieldIdEnum, FieldMetaData> map){ structMap.put(sClass, map); } @@ -53,9 +62,18 @@ public class FieldMetaData implements java.io.Serializable { * Returns a map with metadata (i.e. instances of FieldMetaData) that * describe the fields of the given class. * - * @param sClass The TBase class for which the metadata map is requested + * @param sClass The TBase class for which the metadata map is requested. It is not + * guaranteed that sClass will have been statically initialized before + * this method is called. A racy call to + * {@link FieldMetaData#addStructMetaDataMap(Class, Map)} from a different + * thread during static initialization of the Thrift class is possible. */ - public static synchronized Map<? extends TFieldIdEnum, FieldMetaData> getStructMetaDataMap(Class<? extends TBase> sClass){ + public static Map<? extends TFieldIdEnum, FieldMetaData> getStructMetaDataMap( + Class<? extends TBase> sClass) { + // Note: Do not use synchronized on this method declaration - it leads to a deadlock. + // Similarly, do not trigger sClass.newInstance() while holding a lock on structMap, + // it will lead to the same deadlock. + // See: https://issues.apache.org/jira/browse/THRIFT-5430 for details. if (!structMap.containsKey(sClass)){ // Load class if it hasn't been loaded try{ sClass.newInstance(); diff --git a/lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java b/lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java index bce02c724..333f4eece 100644 --- a/lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java +++ b/lib/javame/src/org/apache/thrift/meta_data/FieldMetaData.java @@ -46,12 +46,19 @@ import java.util.Hashtable; * This class is used to store meta data about thrift fields. Every field in a * a struct should have a corresponding instance of this class describing it. * + * The meta data is registered by ALL Thrift struct classes via a static {...} + * initializer block in the generated Thrift code. + * + * Since different threads could be initializing different Thrift classes, calls + * to the public static methods of this class could be racy. + * + * All methods of this class should be made thread safe. */ public class FieldMetaData { public final String fieldName; public final byte requirementType; public final FieldValueMetaData valueMetaData; - private static Hashtable structMap; + private static final Hashtable structMap; static { structMap = new Hashtable(); @@ -63,18 +70,33 @@ public class FieldMetaData { this.valueMetaData = vMetaData; } - public static synchronized void addStructMetaDataMap(Class sClass, Hashtable map){ - structMap.put(sClass, map); + public static void addStructMetaDataMap(Class sClass, Hashtable map){ + synchronized (structMap) { + structMap.put(sClass, map); + } } /** * Returns a map with metadata (i.e. instances of FieldMetaData) that * describe the fields of the given class. * - * @param sClass The TBase class for which the metadata map is requested + * @param sClass The TBase class for which the metadata map is requested. It is not + * guaranteed that sClass will have been statically initialized before + * this method is called. A racy call to + * {@link FieldMetaData#addStructMetaDataMap(Class, Hashtable)} from a different + * thread during static initialization of the Thrift class is possible. */ - public static synchronized Hashtable getStructMetaDataMap(Class sClass){ - if (!structMap.containsKey(sClass)){ // Load class if it hasn't been loaded + public static Hashtable getStructMetaDataMap(Class sClass) { + // Note: Do not use synchronized on this method declaration - it leads to a deadlock. + // Similarly, do not trigger sClass.newInstance() while holding a lock on structMap, + // it will lead to the same deadlock. + // See: https://issues.apache.org/jira/browse/THRIFT-5430 for details. + boolean isThriftStructStaticallyInitialized = false; + synchronized (structMap) { + isThriftStructStaticallyInitialized = structMap.containsKey(sClass); + } + + if (!isThriftStructStaticallyInitialized){ // Load class if it hasn't been loaded try{ sClass.newInstance(); } catch (InstantiationException e){ @@ -83,6 +105,8 @@ public class FieldMetaData { throw new RuntimeException("IllegalAccessException for TBase class: " + sClass.getName() + ", message: " + e.getMessage()); } } - return (Hashtable) structMap.get(sClass); + synchronized (structMap) { + return (Hashtable) structMap.get(sClass); + } } } |