[rfc][icedtea-web] Cache Locking Tests : Potential Deadlock Fix

[Jie Kang]

Hello,

After some more suggestions from Omair, here is a revised and hopefully decent patch ^^

A CountDownLatch has been used in place of previous 'countdown-like' implementation and timeouts have been added so the tests won't cause any problems (apart from failing) if they somehow run forever. 


Thanks for all the help!


Regards,

----- Original Message -----
> Hello
> 
> 
> Please ignore the previous message containing
> 'itw-cachelock-deadlock-2.patch'. After Omair's review I've improved it a
> bit. More comments in-line.
> 
> ----- Original Message -----
> > * Jie Kang <jkang at redhat.com> [2014-09-23 11:09]:
> > > Jiri reported deadlock occurring in the unit tests for
> > > CacheLRUWrapper. This most likely occurred in the multi-threaded test
> > > where the parent thread sleeps for 2 seconds, before attempting to
> > > wake the sleeping children. If the children have not completed their
> > > tasks within two seconds, they will not be waiting for the parent
> > > thread to wake them, and a deadlock can occur.
> > 
> > I am not sure it counts as a deadlock; just sleeping threads that never
> > wake up. Don't deadlocks require circular wait?
> 
> Ah correct.
> 
> > 
> > > This issue could also occur in CacheEntry, which has a multi-threaded
> > > test that functions similarly. This patch resolves the problem. Sorry
> > > for the mistake.
> > 
> > This is the classic lost-notify problem, right? Why not use a Semaphore?
> > 
> > > +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java
> > 
> > > +        //Would like thread a to acquire lock first
> > > +        Thread.sleep(1000l);
> > > +
> > 
> > Instead of sleeping (and making unit tests take longer) please use a
> > semaphore to signal the readiness.
> > 
> > >          Thread b = new Thread(new WriteWorker(5, entry));
> > >          b.start();
> > >  
> > > -        Thread.sleep(2000l);
> > > +        //Wait for both children to signal (integer increments from 0,
> > > to
> > > 2)
> > > +        while (childSignal.get() < 2) {
> > > +            Thread.sleep(1000l);
> > > +        }
> > >  
> > > -        synchronized (listener) {
> > > -            num = 1;
> > > -            listener.notifyAll();
> > > +        while (parentSignal.get() != 1) {
> > > +            synchronized (listener) {
> > > +                parentSignal.set(1);
> > > +                listener.notifyAll();
> > > +            }
> > 
> > Just to clarify, you want both threads to reach a point in their
> > execution cycle and wait there until both have reached that same point?
> > This sounds like a job for a CountDownLatch (or a Barrier).
> 
> Thanks. I didn't know of CountDownLatch but it has fit what I am trying to do
> perfectly. I guess I can treat this as good experience of writing my own
> poor-man's CountDownLatch... x-x
> 
> 
> The new patch is attached and is much cleaner.
> 
> 
> > 
> > >              try {
> > > -                boolean result = entry.tryLock();
> > > +                entry.tryLock();
> > >                  entry.setLastModified(input);
> > > -                result = entry.store();
> > > -                executorService.execute(new WriteRunnable(":" + input +
> > > ":" + result));
> > > -                while (num == 0) {
> > > +                boolean result = entry.store();
> > > +                synchronized (out) {
> > > +                    out.println(":" + input + ":" + result);
> > > +                    out.flush();
> > > +                }
> > > +                childSignal.getAndIncrement();
> > > +                while (parentSignal.get() == 0) {
> > >                      synchronized (listener) {
> > >                          listener.wait();
> > >                      }
> > 
> > This code calls `entry.unlock()` unconditionally, even if the
> > `entry.tryLock()` fails. This violates the contract of `unlock()`.
> 
> At the moment the unlock() doesn't care if the caller has the lock or not. If
> the caller has the lock, it unlocks, otherwise it does nothing.
> The underlying lock is a FileLock + ReentrantLock. FileLock chooses to do
> nothing on invalid unlocks [1] whereas ReentrantLocks throw an exception
> [2]. The function that calls ReentrantLock's unlock chooses to check for
> lockholder before calling unlock, and simply returns if it is incorrect. [3]
> 
> [1]
> http://docs.oracle.com/javase/7/docs/api/java/nio/channels/FileLock.html#release()
> [2]
> http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantLock.html#unlock()
> [3] LockedFile.java
> public void unlock() throws IOException {
>         if (JNLPRuntime.isWindows() ||
>         !this.threadLock.isHeldByCurrentThread()) {
>             return;
>         }
>         ...
> 
> Should this be changed? I am thinking of making all the void functions return
> booleans so callers can tell if locks/unlocks are successful, but the
> original code was all void so I was reluctant.
> 
> > 
> > Thanks,
> > Omair
> > 
> > --
> > PGP Key: 66484681 (http://pgp.mit.edu/)
> > Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681
> > 
> 
> --
> 
> Jie Kang

-- 

Jie Kang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-cachelock-deadlock-4.patch
Type: text/x-patch
Size: 13710 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140924/5c8efb51/itw-cachelock-deadlock-4-0001.patch>