[rfc][icedtea-web] CacheEntry Locking Fix
Jie Kang
jkang at redhat.com
Wed Aug 20 14:16:02 UTC 2014
----- Original Message -----
> >
> > 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
I did code search for all unlocks. There is now only one place where it is not in finally block:
function initializeResource in ResourceTracker.java
CacheEntry entry = new CacheEntry(resource.getLocation(), resource.getRequestVersion());
entry.lock();
try {
.
.
.
CacheEntry newEntry = new CacheEntry(resource.getLocation(), resource.getRequestVersion());
newEntry.lock();
entry.unlock(); //Do you think this should be in finally block?
entry = newEntry;
.
.
.
} finally {
entry.unlock(); //Technically has a finally unlock here so should be okay.
}
> > 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.
Okay.
>
> > }
> 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?
For unlock: if lock is NOT held by current thread (and process), it cannot be unlocked, so we return. I put this together with JNLPRuntime.isWindows since it also just returns.
>
>
> This locking have to work in process-way not only thread way. Otherwise it
The process-lock is done in LockedFile.java for us. It does both ReentrantLock (thread lock) + FileLock (process lock).
> 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.
Ah you are right. I have corrected it in attached patch.
>
> > @@ -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?
First test (multi-write) fails.
Reason is:
Before patch: Writing did not care if you had lock or not. Even if you have lock, anybody can write.
After patch: You must have lock to write.
Second test (write + read) passes.
Regards,
>
>
> J.
>
--
Jie Kang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-cache-lock-3.patch
Type: text/x-patch
Size: 25688 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140820/9fe935a6/itw-cache-lock-3-0001.patch>
More information about the distro-pkg-dev
mailing list