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

[Omair Majid]

* Jie Kang <jkang at redhat.com> [2014-09-24 17:07]:
> 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!

You are welcome!

> > ----- Original Message -----
> > [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.

FWIW, I think it should be changed to throw an error if the lock is not
held. If a code implements methods from the Lock interface, it should
try and follow the semantics (where possible) closely too. If it does
not follow them, it should at least be documented.

> +    //Check if input string contains only one instance of given substring
> +    private boolean containsSingle(String in, String substr) {

Call the method something like `stringContainsOnlySingleIntance(String
input, String substring)` and you wont need the comment :)

> +        int start = in.indexOf(substr); //first match

Maybe call it `int firstMatch`, then?

> +        String restOfString = in.substring(start+substr.length());
> +
> +        return !substr.contains(restOfString );
>      }

Another implementation might be:

    int firstIndex = in.indexOf(substr);
    int lastIndex = in.lastIndexOf(substr);
    boolean containsString = firstIndex != -1;
    boolean onlyOneInstance = firstIndex == lastIndex;
    return constainsString && onlyOneInstance;

> +            //Wait for writers to be done;
> +            try {
> +                writerSignal.await();

The comment makes me think the latch should be named `writersDone`.

> +            } catch (InterruptedException e) {
> +                e.printStackTrace();

Can you make this fail the test?

> +            }
>              long lastModified = entry.getLastModified();
> -            executorService.execute(new WriteRunnable(":read:" + lastModified));
> +            synchronized (out) {
> +                out.println(":read:" + lastModified);
> +                out.flush();
> +            }

I am a little confused here. When you run this test, does the
`writerSignal.await()` mean that the writer is done and (possibly!)
unlocked the lock? What is this test testing in that case?

> +            //Let people know reading is done

There's nothing that does what the comment says :(

> +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheLRUWrapperTest.java

> @@ -99,36 +93,35 @@
>  
>          final File cacheIndexFile = new File(clw.cacheDir + File.separator + cacheIndexFileName);
>          cacheIndexFile.delete();
> -        try{
> -        
> -        int noLoops = 1000;
> +        try {
> +            int noLoops = 1000;
>  
> -        long time[] = new long[noLoops];
> +            long time[] = new long[noLoops];
>  
> -        clw.lock();
> -        clearCacheIndexFile();
> +            clw.lock();
> +            clearCacheIndexFile();
>  
> -        fillCacheIndexFile(noEntriesCacheFile);
> -        clw.store();
> +            fillCacheIndexFile(noEntriesCacheFile);
> +            clw.store();
>  
> -        // FIXME: wait a second, because of file modification timestamp only provides accuracy on seconds.
> -        Thread.sleep(1000);
> +            // FIXME: wait a second, because of file modification timestamp only provides accuracy on seconds.
> +            Thread.sleep(1000);
>  
> -        long sum = 0;
> -        for(int i=0; i < noLoops - 1; i++) {
> -            time[i]= System.nanoTime();
> -            clw.load();
> -            time[i+1]= System.nanoTime();
> -            if(i==0)
> -                continue;
> -            sum = sum + time[i] - time[i-1];
> -        }
> -        
> -        double avg = sum / time.length;
> -        ServerAccess.logErrorReprint("Average = " + avg + "ns");
> +            long sum = 0;
> +            for(int i=0; i < noLoops - 1; i++) {
> +                time[i]= System.nanoTime();
> +                clw.load();
> +                time[i+1]= System.nanoTime();
> +                if(i==0)
> +                    continue;
> +                sum = sum + time[i] - time[i-1];
> +            }
>  
> -        // wait more than 100 microseconds for noLoops = 1000 and noEntries=1000 is bad
> -        assertTrue("load() must not take longer than 100 ??s, but took in avg " + avg/1000 + "??s", avg < 100 * 1000);
> +            double avg = sum / time.length;
> +            ServerAccess.logErrorReprint("Average = " + avg + "ns");
> +
> +            // wait more than 100 microseconds for noLoops = 1000 and noEntries=1000 is bad
> +            assertTrue("load() must not take longer than 100 ??s, but took in avg " + avg/1000 + "??s", avg < 100 * 1000);

This is just an indentation change, right? I don't see code changes.

> +    //Check if input string contains only one instance of given substring
> +    private boolean containsSingle(String in, String substr) {

A duplicate method? Please put it in some shared code.

Thanks,
Omair

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