[rfc][icedtea-web] CacheEntry Locking Fix
Jiri Vanek
jvanek at redhat.com
Wed Aug 20 14:55:49 UTC 2014
ok to head then.
On 08/20/2014 04:16 PM, Jie Kang wrote:
>
>
> ----- 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.
>>
>
More information about the distro-pkg-dev
mailing list