RFR(xxs): 8213236: A partial removed/deleted JavaThread cannot transition

[Daniel D. Daugherty]

On 11/1/18 7:57 AM, Robbin Ehn wrote:
> Hi all please review,
>
> In 8212933 we moved the cancellation of a handshake to the VM thread, 
> for sanity
> an assert was added to make sure the JavaThread do not cancel. 
> (technically this
> works also, but to keep a simple model it should be done by the VM 
> thread)
>
> A JavaThread that is terminated but not yet free should not follow the 
> safepoint
> protocol. Since the VM thread running the safepoint cannot see such a 
> thread
> it's confusing and error-prone doing safepoint checks and transitions 
> from a
> thread not 'attached' to the VM.
>
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8213236

I updated the bug to be a bit more clear about what the problem is.
Please let me know if my analysis is correct or not...


>
> Code:
> diff -r 13ad2bfee99e src/hotspot/share/runtime/vmThread.cpp
> --- a/src/hotspot/share/runtime/vmThread.cpp    Thu Nov 01 09:57:57 
> 2018 +0100
> +++ b/src/hotspot/share/runtime/vmThread.cpp    Thu Nov 01 12:40:47 
> 2018 +0100
> @@ -319,7 +319,9 @@
>  // Notify the VMThread that the last non-daemon JavaThread has 
> terminated,
>  // and wait until operation is performed.
>  void VMThread::wait_for_vm_thread_exit() {
> -  { MutexLocker mu(VMOperationQueue_lock);
> +  assert(Thread::current()->is_Java_thread(), "Should be a JavaThread");
> +  assert(((JavaThread*)Thread::current())->is_terminated(), "Should 
> be off threadslist");

This should be "should be terminated". is_terminated()
doesn't necessarily mean the JavaThread is not on the
global ThreadsList.


> +  { MutexLockerEx mu(VMOperationQueue_lock, 
> Mutex::_no_safepoint_check_flag);

Yup. I agree with this addition of Mutex::_no_safepoint_check_flag.

However, I think there is one more:

src/hotspot/share/runtime/thread.cpp:
   before_exit(thread);

   thread->exit(true);

   // Stop VM thread.
   {
     // 4945125 The vm thread comes to a safepoint during exit.
     // GC vm_operations can get caught at the safepoint, and the
     // heap is unparseable if they are caught. Grab the Heap_lock
     // to prevent this. The GC vm_operations will not be able to
     // queue until after the vm thread is dead. After this point,
     // we'll never emerge out of the safepoint before the VM exits.

     MutexLocker ml(Heap_lock);

     VMThread::wait_for_vm_thread_exit();

You fixed the one in wait_for_vm_thread_exit(), but I think
that this: "MutexLocker ml(Heap_lock)" is also a problem.

Dan



> _should_terminate = true;
>      VMOperationQueue_lock->notify();
>    }
>
> Passes t1-2.
>
> Thanks Robbin