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

[Omair Majid]

* 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?

> 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).

>              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()`.

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681