RFR (XS): 8022595: JSR292: deadlock during class loading of MethodHandles, MethodHandleImpl & MethodHandleNatives

[Vladimir Ivanov]

David,

To be accurate, ExceptionMark terminates the process both in debug & 
product VM when pending exception is present:

src/share/vm/utilities/exceptions.cpp:
ExceptionMark::~ExceptionMark() {
   if (_thread->has_pending_exception()) {
     Handle exception(_thread, _thread->pending_exception());
     _thread->clear_pending_exception(); // Needed to avoid infinite 
recursion
     if (is_init_completed()) {
       exception->print();
       fatal("ExceptionMark destructor expects no pending exceptions");
     } else {
       vm_exit_during_initialization(exception);
     }
   }
}

For stand-alone Java application such behavior is reasonable, but in the 
case VM is embedded into another application I don't think VM 
initialization failure is fatal per se.

Best regards,
Vladimir Ivanov

On 8/21/13 4:48 AM, David Holmes wrote:
> On 21/08/2013 8:35 AM, Coleen Phillimore wrote:
>>
>> On 08/20/2013 06:24 PM, Coleen Phillimore wrote:
>>>
>>>
>>> Vladimir,
>>>
>>> You are right, it's broken.  Calvin is working on a similar issue in
>>> bug https://jbs.oracle.com/bugs/browse/JDK-8020675.
>>
>> Reading more, I think Calvin's issue is similar but different.  I will
>> file a new bug for the Threads::create_vm() EXCEPTION_MARK.
>
> I think the intent is that we never, ever expect to get any exceptions
> during VM initialization unless we have a significant bug. The
> EXCEPTION_MARK detects that bug in debug builds.
>
> I have a vague recollection of the CHECK_0 "bug" being discussed when I
> was in runtime.
>
> David
>
>> Coleen
>>
>>>
>>> I think you should check in your code and we'll fix all the
>>> CHECK_0/EXCEPTION_MARK problems at once.
>>>
>>> Thanks for checking this out.
>>> Coleen
>>>
>>> On 08/20/2013 05:55 PM, Vladimir Ivanov wrote:
>>>> Coleen,
>>>>
>>>> Thank you for the review.
>>>> It doesn't cause a deadlock anymore because class loading is
>>>> guaranteed to be performed in singe-threaded mode.
>>>>
>>>> Regarding CHECK_0 -> CHECK_(JNI_ERR) change you propose, there's one
>>>> problem. All pending exceptions are fatal right now: even if it
>>>> returns JNI_OK, there's an ExceptionMark on the stack which crashes
>>>> VM if any pending exceptions are present.
>>>>
>>>> Considering that JNI_CreateJavaVM uses Thread::create_vm and failure
>>>> during VM initialization can crash the whole app, I don't think such
>>>> behavior is correct.
>>>>
>>>> So, I have 2 questions here:
>>>>     1) what behavior do we prefer?
>>>>     2) if we want to change current behavior, doesn't it deserve a
>>>> separate fix?
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> On 8/20/13 11:02 PM, Coleen Phillimore wrote:
>>>>>
>>>>> Vladimir,
>>>>>
>>>>> The code that you added is copied from the initialization code
>>>>> above.  I
>>>>> think they have a problem.  If initialization fails, there is a
>>>>> CHECK_0
>>>>> at the end of the call.  CHECK_0 returns JNI_OK to the caller, which
>>>>> goes ahead with the initialization.
>>>>>
>>>>> Can you change all of these to CHECK_(JNI_ERR) so that the caller can
>>>>> deal with the error?  It would be good to see if the caller does the
>>>>> right thing with a JNI_ERR.
>>>>>
>>>>> Otherwise, I think the code is fine if the initialization of these
>>>>> classes doesn't cause deadlocks in their new place.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 08/20/2013 01:05 PM, Vladimir Ivanov wrote:
>>>>>> http://cr.openjdk.java.net/~vlivanov/8022595/webrev.01/
>>>>>> 201 lines changed: 201 ins; 0 del; 0 mod;
>>>>>>
>>>>>> Some classes in j.l.invoke have circular dependencies between them
>>>>>> which leads to deadlocks during class initialization when multiple
>>>>>> threads exercise JSR292 functionality.
>>>>>>
>>>>>> JSR292 core classes are among "well-known" classes to VM and they are
>>>>>> preloaded during VM startup (see
>>>>>> SystemDictionary::initialize_preloaded_classes). However, it doesn't
>>>>>> force preloaded classes to be initialized (doesn't execute static
>>>>>> initializers). Thus, these classes should be explicitly
>>>>>> pre-initialized in order to avoid deadlocks.
>>>>>>
>>>>>> Based on my observations of possible deadlock scenarios, I chose 3
>>>>>> classes to pre-initialize:
>>>>>>     - MethodHandle
>>>>>>     - MemberName
>>>>>>     - MethodHandleNatives
>>>>>>
>>>>>> I placed the code right after compilers initialization because during
>>>>>> method resolution for signature polymorphic intrinsics, compilation
>>>>>> tasks are issued (see SystemDictionary::find_method_handle_intrinsic
>>>>>> for details) and they are missed if compiler subsystem isn't ready
>>>>>> yet.
>>>>>>
>>>>>> Testing: failing test, regression test, JPRT, jdk/test/j/l/invoke,
>>>>>> vm.mlvm.testlist, octane.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>
>>>
>>