You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@thrift.apache.org by je...@apache.org on 2021/06/23 18:44:10 UTC
[thrift] branch master created (now 58fa7b4)
This is an automated email from the ASF dual-hosted git repository.
jensg pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/thrift.git.
at 58fa7b4 THRIFT-5430: Fix deadlock triggered by FieldMetaData.class.
This branch includes the following new commits:
new 58fa7b4 THRIFT-5430: Fix deadlock triggered by FieldMetaData.class.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[thrift] 01/01: THRIFT-5430: Fix deadlock triggered by
FieldMetaData.class.
Posted by je...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jensg pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/thrift.git
commit 58fa7b4610dc78cad434fcdc535c79082a53160b
Author: Divye Kapoor <dk...@pinterest.com>
AuthorDate: Thu Jun 17 10:10:59 2021 -0700
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 445f7e4..7534390 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 bce02c7..333f4ee 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);
+ }
}
}