The fix for the following bug may need more work:

JDK-8047720 Xprof hangs on Solaris

I've seen two separate e-mails about the fix for 8047720 in less than a week.

First was this e-mail sent directly to me:

On 1/28/15 12:28 PM, Alexander Garthwaite wrote:
> Subject: question about fix for PeriodicTask_lock/WatcherThread deadlock
> Dan,
>     Apologies for writing directly. I am backporting your fix to a version
> 7 copy of the OpenJDK used at Twitter. The fix (which looks good overall)
> is to try the lock and only wake the WatcherThread if we get it. It appears
> there is still a small race (which may not matter) if we have the following
> sequence:
>    *  the WatcherThread has the lock
>    *  some other thread, T, calls stop() and fails to get the lock
>    *  the WatcherThread unlocks the PeriodicTask_lock, still sees
> _should_stop as false and parks
>    *  T doesn't have the lock and so exits stop without doing an unpack
> This would allow the WatcherThread to park without seeing that it should stop.
> I was wondering if the following small change would fix that:
>  void WatcherThread::stop() {
> +  // Get the PeriodicTask_lock if we can. If we cannot, then the
> +  // WatcherThread is using it and we don't want to block on that lock
> +  // here because that might cause a safepoint deadlock depending on
> +  // what the current WatcherThread tasks are doing.
> +  bool have_lock = PeriodicTask_lock->try_lock();
> +
> +  _should_terminate = true;
> +  OrderAccess::fence();  // ensure WatcherThread sees update in main loop
>    // Retry the lock if we don't have it. This handles the case where
>    // the WatcherThread stopped using and parked without seeing
>    // _should_terminate set to true.
>    if (!have_lock) {
>      have_lock = PeriodicTask_lock->try_lock();
>    }
> +  if (have_lock) {
>      WatcherThread* watcher = watcher_thread();
> +    if (watcher != NULL) {
> +      // If we managed to get the lock, then we should unpark the
> +      // WatcherThread so that it can see we want it to stop.
>         watcher->unpark();
> +    }
> +
> +    PeriodicTask_lock->unlock();
>    }
> (Ignore the leading '+'s. They're marking your patch.) Thanks!
>                                -- Alex


Second was this e-mail sent on 2015.02.03 to serviceability-dev@openjdk.java.net:

On 2/3/15 12:00 PM, Carsten Varming wrote:
> Subject: Patch: Clean up fix for JDK-8047720
> Greetings all,
> I was recently introduced to the deadlock described in JDK-8047720 and fixed in JDK9. The fix seems a little messy to me, and it looks like it left some very short races in the code. So I decided to clean up the code. See attached diff.
> The change takes a step back and acquires PeriodicTask_lock in WatcherThread::stop (like before the fix in JDK9), but this time safepoints are allowed to proceed when acquiring PeriodicTask_lock, preventing the deadlock.
> Let me know what you think,
> Carsten

Carsten's patch attachment:

diff -r 8ff882030755 src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp	Wed Jan 28 16:45:35 2015 -0800
+++ b/src/share/vm/runtime/thread.cpp	Tue Feb 03 13:52:32 2015 -0500
@@ -1318,24 +1318,20 @@
 }
 
 void WatcherThread::stop() {
-  // Get the PeriodicTask_lock if we can. If we cannot, then the
-  // WatcherThread is using it and we don't want to block on that lock
-  // here because that might cause a safepoint deadlock depending on
-  // what the current WatcherThread tasks are doing.
-  bool have_lock = PeriodicTask_lock->try_lock();
-
   _should_terminate = true;
-  OrderAccess::fence();  // ensure WatcherThread sees update in main loop
-
-  if (have_lock) {
+  {
+    // Let safepoints happen to avoid the deadlock:
+    // The VM thread has Threads_lock and waits for Java threads to report back for a safepoint.
+    // The watcher thread has PeriodicTask_lock and tries to acquire Threads_lock.
+    // A Java thread executes WatcherThread::stop, tries to acquire PeriodicTask_lock,
+    // and holds up the VM thread by not reaching a safepoint.
+    MutexLocker mu(PeriodicTask_lock);
+
     WatcherThread* watcher = watcher_thread();
     if (watcher != NULL) {
-      // If we managed to get the lock, then we should unpark the
-      // WatcherThread so that it can see we want it to stop.
+      // Unpark the WatcherThread so that it can see that it should terminate.
       watcher->unpark();
     }
-
-    PeriodicTask_lock->unlock();
   }
 
   // it is ok to take late safepoints here, if needed