Dear Daniel, Great news.
I was reading your comments on JDK-8072439 and just want to point out the other race that my patch fixed: WatcherThread::run sees !_should_terminate, and calls sleep. WatcherThread::stop takes PeriodicTask_lock, sets _should_terminate, notify PeriodicTask_lock, and release the lock. Sleep wait forever on PeriodicTask_lock. Notice that the wait forever happens only if there are no periodic tasks scheduled, so this might not be reproducible (even with suitable sleeps), yet it is a lurking bug. Let me know if there is anything I can do to help, Carsten On Tue, Feb 17, 2015 at 12:43 PM, Daniel D. Daugherty < daniel.daughe...@oracle.com> wrote: > Carsten. > > I've started diving into this issue. Please see the updates > that I've been making to: > > JDK-8072439 fix for 8047720 may need more work > https://bugs.openjdk.java.net/browse/JDK-8072439 > > Dan > > > On 2/10/15 3:18 PM, Carsten Varming wrote: > > Dear David, > > Any news regarding this fix? Anything I can do to help? > > Best wishes, > Carsten > > On Wed, Feb 4, 2015 at 12:54 PM, Carsten Varming <varm...@gmail.com> > wrote: > >> Dear David, >> >> I took a look at all occurrences of PeriodicTask_lock. We are in luck: >> >> PeriodicTask::enroll and PeriodicTask::disenroll check for safepoints >> when acquiring PeriodicTask_lock. >> >> PeriodicTask::time_to_wait and PeriodicTask::real_time_tick are called >> only by the watcher thread. >> >> WatcherThread::unpark is used only in contexts where PeriodicTask_lock >> is owned by the caller. >> >> WatcherThread::sleep is called only by the watcher thread. >> >> I took another look at WatcherThread::sleep and realized that there is >> a path from acquiring PeriodicTask_lock to waiting on the lock where >> _should_terminate is not checked. I added that to the minimal fix attached. >> >> Looking at these methods made me want to clean up a little more. See >> better.patch attached. >> >> I feel pretty good about the patch with the limited usage of >> no_safepoint_check_flag with PeriodicTask_lock. >> >> Carsten >> >> >> On Tue, Feb 3, 2015 at 11:28 PM, David Holmes <david.hol...@oracle.com> >> wrote: >> >>> On 4/02/2015 1:28 PM, Carsten Varming wrote: >>> >>>> Dear David Holmes, >>>> >>>> Please see inline response, >>>> >>>> On Tue, Feb 3, 2015 at 9:38 PM, David Holmes <david.hol...@oracle.com >>>> <mailto:david.hol...@oracle.com>> wrote: >>>> >>>> On 4/02/2015 5:00 AM, Carsten Varming wrote: >>>> >>>> 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. >>>> >>>> >>>> It isn't obvious that blocking for a safepoint whilst in this method >>>> will always be a safe thing to do. That would need to be examined in >>>> detail - potential interactions can be very subtle. >>>> >>>> >>>> Absolutely true. For reference, the pattern is repeated with the >>>> Terminator_lock a few lines below. The pattern is also used in >>>> Threads::destroy_vm before and after calling before_exit, and the java >>>> shutdown hooks are called right before the call to before_exit. So there >>>> is a lot of evidence that safepoints are allowed to happen in this >>>> context. >>>> >>> >>> The thread calling before_exit is a JavaThread so of course it >>> participates in safepoints. The issue is whether the interaction between >>> that thread and the WatcherThread, via the PeriodicTask_lock, can allow for >>> the JavaThread blocking at a safepoint whilst owning that lock. If another >>> JavaThread can try to lock it without a safepoint check then we can get a >>> deadlock. >>> >>> Cheers, >>> David >>> >>> >>> Thank you for taking your time to look at this, >>>> Carsten >>>> >>>> >>>> David >>>> >>>> >>>> Let me know what you think, >>>> Carsten >>>> >>>> >>>> >> > >
