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

[David Holmes]

On 21/08/2013 3:55 PM, Vladimir Ivanov wrote:
> David,
>
> To be accurate, ExceptionMark terminates the process both in debug &
> product VM when pending exception is present:

Okay.

> 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.

No it isn't, but we have a dozen different things that cause VM 
initialization to abort the process when it shouldn't. :( You would 
think this would have been handled from the beginning but ... <sigh>

I'm not saying don't fix this, just that it is one of many things that 
need fixing in this area. And there are other places where we have 
EXCEPTION_MARKS placed right before code that throws an exception. :(

Cheers,
David

> 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
>>>>>>
>>>>
>>>