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

[Jie Kang]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-cachelock-deadlock-3.patch
Type: text/x-patch
Size: 13940 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140924/d00066f6/itw-cachelock-deadlock-3.patch>