From 58fa7b4610dc78cad434fcdc535c79082a53160b Mon Sep 17 00:00:00 2001 From: Divye Kapoor Date: Thu, 17 Jun 2021 10:10:59 -0700 Subject: 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. --- .../org/apache/thrift/meta_data/FieldMetaData.java | 28 +++++++++++++--- .../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, Map> structMap; + private static final Map, Map> structMap; static { - structMap = new HashMap, Map>(); + structMap = new ConcurrentHashMap, Map>(); } 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 sClass, Map map){ + public static void addStructMetaDataMap(Class sClass, Map 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 getStructMetaDataMap(Class sClass){ + public static Map 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. 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); + } } } -- cgit v1.2.1