Details
-
Bug
-
Status: Closed
-
Major
-
Resolution: Fixed
-
0.6, 0.6.1, 0.7, 0.8, 0.9, 0.9.1, 0.9.2, 0.9.3, 0.9.3.1, 0.10.0, 0.11.0, 0.12.0, 0.13.0, 0.14.0, 0.14.1
-
None
-
AdoptJDK 1.8 (Java 8) ; Linux, AWS machines.
Deadlock is environment independent and occurs in conformance with the JRE 8 spec.
-
Patch Available
-
Patch, Important
Description
We (Pinterest) hit the following deadlock during JVM classloading of Thrift classes. The root cause was triggering sClass.newInstance() while holding the synchronized lock on FieldMetaData.class::getStructMetaDataMap(..)
Here's the stacktraces of interest:
Thread 1:
Stack Trace is: at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) // stuck here for > 30 mins at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:423) at java.lang.Class.newInstance(Class.java:442) // creating an object via reflection. at org.apache.thrift.meta_data.FieldMetaData.getStructMetaDataMap(FieldMetaData.java:61)- locked java.lang.Class@346242bb // Lock held on FieldMetaData.class at com.pinterest.commons.thrift.ThriftStructDescriptor.<init>(ThriftStructDescriptor.java:56) at com.pinterest.commons.thrift.ThriftStructDescriptor.getDescriptor(ThriftStructDescriptor.java:38)
Thread 2:
Stack Trace is: at org.apache.thrift.meta_data.FieldMetaData.addStructMetaDataMap(FieldMetaData.java:49) - blocked on java.lang.Class@346242bb // Lock held by Thread 1. at com.pinterest.schemas.search.query.NavboostScore.<clinit>(NavboostScore.java:146) at com.pinterest.schemas.search.query.NavboostScores.read(NavboostScores.java:334) at com.pinterest.schemas.search.query.SupplementaryTerm.read(SupplementaryTerm.java:1209) at com.pinterest.schemas.search.query.SupplementaryTerms.read(SupplementaryTerms.java:335) at com.pinterest.schemas.search.query.QueryJoin.read(QueryJoin.java:6514) at com.pinterest.pinpin.thrift.SerializedResourceContainer.read(SerializedResourceContainer.java:2867) at org.apache.thrift.TDeserializer.deserialize(TDeserializer.java:69) // General Thrift deserialize.
Here's the code of interest:
Thread 1 has the following lock acquisition order:
FieldMetaData.class::getStructMetaDataMap
-> lock FieldMetaData.class (LOCK 1)
-> sClass.newInstance()
-> load sClass
-> JVM init_lock (native) lock and release. (LOCK 2)
-> waiting for Thread 2 to complete static initialization and notify thread 1.
Thread 2 has the following lock acquisition order:
sClass.newInstance() (from new ThriftObject() / deserialization)
-> load sClass
-> static initialization
-> try locking FieldMetaData.class when calling FieldMetaData.addStructMetaDataMap (WAIT on LOCK 1 which is never released)
---> does not ever trigger the next step:
---> Notify Thread 1 that static initialization of the class has completed (releasing LOCK 2 on Thread 1 which will eventually release LOCK 1 by returning from native code).
Internally, this was a fairly detailed investigation. Would be great if we agree on a Fix approach. This deadlock affects all versions of Thrift since 0.9 which introduced the synchronized keyword. Versions prior to 0.9 are simply thread unsafe (because there's no lock on FieldMetaData's internal HashMap and the two static method calls can race). I didn't check versions below 0.5 but probably this extends all the way back.
This is not an issue in fbThrift, which correctly uses a ConcurrentHashMap and does not take a lock on FieldMetaData. See this code here:
https://github.com/facebook/fbthrift/blob/c0f8ffb345d847df512ae9c404a58379fcd51cb9/thrift/lib/java/src/main/java/com/facebook/thrift/meta_data/FieldMetaData.java vs the buggy code here:
Another alternative approach is to use
synchronized (structMap} {
...
}
instead of the synchronized keyword on the methods.
Please confirm which approach is preferred and we can send a PR on Github.
Here's the original deadlock hypothesis as outlined by Jiahuan Liu:
The hypothesis of the deadlock:
ThriftStructDescriptor.get is used in many places for deserialization and it calls FieldMetaData.getStructMetaDataMap. inside this method, it tries to load the thrift class if the thrift has not been loaded yet. thrift classes call FieldMetaData.addStructMetaDataMap in a static block. so the deadlock may happen when:
- thread A is trying to deserialize -> getStructMetaDataMap -> class loading
- if thread B is already trying to load the same thrift, it requires addStructMetaDataMap to complete but this method is blocked by thread A getStructMetaDataMap
- the class loading in thread A will be blocked by thread B because class loading is also synchronized by java. see this section in java spec:
If the Class object for C indicates that initialization is in progress for C by some other thread, then release LC and block the current thread until informed that the in-progress initialization has completed, at which time repeat this step.
Here's the classloading spec in java 8:
https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.4.2
Thread 1 is stuck in step 2 of the initialization (waiting for Thread 2 to notify in Step 10).
Thread 2 is stuck in step 9 of the initialization and never makes it to step 10.
Precondition: this is the first object creation for that Thrift object and at-least 2 threads are racing to create the object, one of which holds a lock on FieldMetaData.class.
Please reach out if this is unclear. This hypothesis was validated by reading through the native JVM code on the class load -> static_initialization path.
Here's the native JVM code if you're interested: InstanceKlass::initialize_impl
Attachments
Issue Links
- links to
