THRIFT-5430: Fix deadlock triggered by FieldMetaData.class.

Details of the deadlock are in the ticket.
This PR addresses the deadlock by limiting the scope of the locks
acquired in FieldMetaData.java to only protect calls to the structMap
hashtable. No locks should be held when the call to sClass.newInstance()
is in progress.

Tested: Regular CI builds should pass.

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