[rfc][icedtea-web] CacheEntry Locking Fix

[Jiri Vanek]

>
> Thanks for the review :D
Sorry:( Not in own skin for some time;(

>
>> >Two nits
>> >   - you should always unlock in finally clause.
> I've searched through the code and moved all applicable unlocks into finally clause.
>
>> >   - the unittests - the tests you had in previouspatch (the ignored two) are
>> >   not applicable?
>> >    - if so,please, add
>> >    - the unittest testing the locking feature is actually missing
>> >
>> >
>> >something like start thread one, lock and write write write. Start thread two
>> >and try to read
>> >(should be ok) start thread three and try to write - shouldnot write a single
>> >bite. Pelase add those
>> >teo tests.
> I've added two tests, one for testing Multiple Writes While Locked and one for Reading While Locked.
>
>

Still few nits.:

You added one final clausule - are those all usages of unlock? That does not mean all usages needs 
to be in finally block, but .... they probably should
> Regards,
>
>> >
>> >
>> >J.
>> >
>> >Please add those two tests.
>> >
>> >
>> >
>> >
> -- Jie Kang
>
>
> itw-cache-lock-2.patch
>
>

There are some (actualy a lots) format-only changes (eg imports or similar) Please do your best t5o 
backport those to 1.5.

>   }
tion {
>           if (this.processLock != null) {
>               return;
>           }
> @@ -140,7 +153,7 @@
>        * Unlock access to the file. Lock is reentrant.
>        */
>       public void unlock() throws IOException {
> -        if (JNLPRuntime.isWindows()) {
> +        if (JNLPRuntime.isWindows() || !this.threadLock.isHeldByCurrentThread()) {


This is suspicious. Why so?


This locking have to work in process-way  not only thread way. Otherwise it do not have sense.
>               return;
>           }
>           boolean releaseProcessLock = (this.threadLock.getHoldCount() == 1);
> @@ -163,4 +176,8 @@
>               this.threadLock.unlock();
>           }
>       }
> +
> +    public boolean isHeldByCurrentThread() {
> +        return this.threadLock.isHeldByCurrentThread();
> +    }
>   }
> \ No newline at end of file
> diff --git a/tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java
> +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java


Uf from the patch it is not obvious but are all those tests writing to "sharp and used" cache file?
They should not.

> @@ -41,21 +41,25 @@
>   import static org.junit.Assert.assertFalse;
>   import static org.junit.Assert.assertTrue;
>
> +import java.io.ByteArrayOutputStream;
>   import java.io.File;
>   import java.io.IOException;
> +import java.io.PrintStream;
>   import java.net.MalformedURLException;
>   import java.net.URL;
>   import java.nio.file.Files;
> +import java.util.concurrent.ExecutorService;
> +import java.util.concurrent.Executors;
> +
> +import org.junit.Before;
> +import org.junit.Test;
>
>   import net.sourceforge.jnlp.Version;
>   import net.sourceforge.jnlp.util.PropertiesFile;
>
> -import org.junit.Before;
> -import org.junit.Test;
> -
>   public class CacheEntryTest {
>
> -    /** A custom subclass that allows supplying a predefined cache file */
> +    /** A custom subclass that allows supplying num predefined cache file */
>       static class TestCacheEntry extends CacheEntry {
>           private File cacheFile;
>           public TestCacheEntry(URL location, Version version, File cacheFile) {
> @@ -79,10 +83,20 @@
>       private URL url;
>       private Version version;
>
> +    private ByteArrayOutputStream baos;
> +    private PrintStream out;
> +    private ExecutorService executorService;
> +    Object listener = new Object();
> +
> +    int num = 0;
> +
>       @Before
>       public void setUp() throws MalformedURLException {
>           url = new URL("http://example.com/example.jar");
>           version = new Version("1.0");
> +        baos = new ByteArrayOutputStream();
> +        out = new PrintStream(baos);
> +        executorService = Executors.newSingleThreadExecutor();
>       }
>
>       @Test
> @@ -208,4 +222,126 @@
>           return cachedFile;
>       }
>
> +    @Test
> +    public void testLock() throws IOException {
> +        CacheEntry entry = new CacheEntry(url, version);
> +        entry.lock();
> +        assertTrue(entry.isHeldByCurrentThread());
> +        entry.unlock();
> +    }
> +
> +    @Test
> +    public void testUnlock() throws IOException {
> +        CacheEntry entry = new CacheEntry(url, version);
> +        entry.lock();
> +        entry.unlock();
> +        assertTrue(!entry.isHeldByCurrentThread());
> +    }
> +
> +    @Test
> +    public void testStoreFailsWithoutLock() throws IOException {
> +        CacheEntry entry = new CacheEntry(url, version);
> +        long num = 10;
> +        entry.setLastModified(num);
> +        assertTrue(!entry.store());
> +    }
> +
> +    @Test
> +    public void testStoreWorksWithLocK() throws IOException {
> +        CacheEntry entry = new CacheEntry(url, version);
> +        long num = 10;
> +        entry.setLastModified(num);
> +        entry.lock();
> +        assertTrue(entry.store());
> +        entry.unlock();
> +    }
> +
> +    @Test
> +    public void testMultithreadLockPreventsWrite() throws IOException, InterruptedException {
> +        Thread a = new Thread(new WriteWorker(10));
> +        a.start();
> +        Thread b = new Thread(new WriteWorker(5));
> +        b.start();
> +
> +        Thread.sleep(2000l);
> +
> +        synchronized (listener) {
> +            num = 1;
> +            listener.notifyAll();
> +        }
> +
> +        String out = baos.toString();
> +        assertTrue(out.contains(":10:true\n:5:false"));
> +    }
> +
> +    @Test
> +    public void testMultithreadLockAllowsRead() throws IOException, InterruptedException {
> +        Thread a = new Thread(new WriteWorker(10));
> +        a.start();
> +
> +        Thread.sleep(2000l);
> +
> +        Thread b = new Thread(new ReadWorker());
> +        b.start();
> +
> +        Thread.sleep(2000l);
> +
> +        synchronized (listener) {
> +            num = 1;
> +            listener.notifyAll();
> +        }
> +
> +        String out = baos.toString();
> +
> +        assertTrue(out.contains(":10:true\n:read:10"));
> +    }
> +
> +    private class WriteWorker implements Runnable {
> +        long input;
> +        public WriteWorker(long input) {
> +            this.input = input;
> +        }
> +        @Override
> +        public void run() {
> +            CacheEntry entry = new CacheEntry(url, version);
> +            try {
> +                entry.tryLock();
> +                entry.setLastModified(input);
> +                boolean result = entry.store();
> +                executorService.execute(new WriteRunnable(":" + input + ":" + result));
> +                while (num == 0) {
> +                    synchronized (listener) {
> +                        listener.wait();
> +                    }
> +                }
> +            } catch (Exception e) {
> +                e.printStackTrace();
> +            } finally {
> +                entry.unlock();
> +            }
> +        }
> +    }
> +
> +    private class WriteRunnable implements Runnable {
> +        private String msg;
> +        public WriteRunnable(String msg) {
> +            this.msg = msg;
> +        }
> +
> +        @Override
> +        public void run() {
> +            out.println(msg);
> +            out.flush();
> +        }
> +    }
> +
> +    private class ReadWorker implements Runnable {
> +
> +        @Override
> +        public void run() {
> +            CacheEntry entry = new CacheEntry(url, version);
> +            long lastModified = entry.getLastModified();
> +            executorService.execute(new WriteRunnable(":read:" + lastModified));
> +        }
> +    }
>   }
>


Actually.... Are those two tests failing without the patch itsef?


J.