RFR: 8273608: Deadlock when jcmd of OnError attaches to itself

[Xin Liu]

On Tue, 12 Oct 2021 23:04:22 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> On 10\/12\/21 8\:04 AM\, David Holmes wrote\:
>>> On 12\/10\/2021 4\:15 pm\, Xin Liu wrote\:
>>>> On Tue\, 12 Oct 2021 05\:27\:15 GMT\, David Holmes \<dholmes at openjdk\.org> 
>>>> wrote\:
>>>>
>>>>>> Xin Liu has updated the pull request incrementally with one 
>>>>>> additional commit since the last revision\:
>>>>>>
>>>>>> \?\? Revert JavaThreadInVMAndNative change\.
>>>>>> \?\? \?\? Only change the state to Native if current thread is Java 
>>>>>> Thread and it\'s in VM\.
>>>>>> \?\? Restore thread state after fork\_and\_exec\(\) if it\'s changed\.
>>>>>
>>>>> src\/hotspot\/share\/utilities\/vmError\.cpp line 1659\:
>>>>>
>>>>>> 1657\:\?\?\?\?\?\? VMErrorForceInNative fn\(Thread\:\:current\_or\_null\(\)\)\;
>>>>>> 1658\:\?\?\?\?\?\? if \(os\:\:fork\_and\_exec\(cmd\) \< 0\) \{
>>>>>> 1659\:\?\?\?\?\?\?\?\? out\.print\_cr\(\"os\:\:fork\_and\_exec failed\: \%s \(\%s\=\%d\)\"\,
>>>>>
>>>>> I\'m still concerned about using out\.print\_cr if you are 
>>>>> \_thread\_in\_native \- is it safe\? If it is\, and if \`print\_raw\` is also 
>>>>> safe to execute when in native\, then you could just apply the 
>>>>> VMErrorForceInNative before the while loop\.
>>>>
>>>> I think it\'s safe\. IIUC\, a java thread in Native is safe if we keep 
>>>> track all oops in handles\.
>>>> In that sense\, we prevent them from GC\.\? A fdStream should have 
>>>> nothing to do with oops and GC\.
>>>
>>> Can out\.print\_cr acquire a lock \(e\.g\. ttyLock\?\) \? If so then it could 
>>> block and that would use a ThreadBlockInVM which requires it to be 
>>> \_thread\_in\_vm\. That is the kind of \"safety\" I\'m concerned about\.
>> 
>> ttyLock is taken and released inside of print\_cr\, but it\'s a nosafepoint 
>> lock so would not block\.
>> Coleen
>> 
>>>
>>> David
>>> \-\-\-\-\-
>>>
>
>> ttyLock is taken and released inside of print_cr, but it's a nosafepoint lock so would not block.
> 
> Okay - thanks @coleenp

hi, @dholmes-ora , @tstuefe , 

Thank you for helping me in this PR. Let me drop this one.

This is 10x more harder than I initially thought. `VMError::report_and_die()` could be called from anywhere, anytime. I thought _thread must be in VM state if it is a Java Thread. I was wrong.

Like David pointed out, this patch can only solve "the second possibilities". Without it,  we can claim that "Deadlock when jcmd of OnError attaches to itself" is a feature instead of a bug. But we shouldn't leave undefined behavior to HotSpot for "the 1st set of possibilities".  Further, transitioning current Java Thread to Native can't guarantee it's safepoint safe either. It's still up to the last frame.


static bool safepoint_safe_with(JavaThread *thread, JavaThreadState state) {
  switch(state) {
  case _thread_in_native:
    // native threads are safe if they have no java stack or have walkable stack
    return !thread->has_last_Java_frame() || thread->frame_anchor()->walkable();
...


I don't like nondeterministic behavior, which leaves more questions than answers. We can revisit it later if we have a better solution.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5590