[rfc][icedtea-web] Cache Locking Tests : Potential Deadlock Fix
Jie Kang
jkang at redhat.com
Fri Sep 26 20:41:07 UTC 2014
----- Original Message -----
> * 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.
Hello,
Right. I will document it for now and then look at altering it to throw an error in another changeset. Technically the Lock interface specifies that actions taken on invalid unlocks is up to the implementation [1]
[1] http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Lock.html#unlock()
>
> > + //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;
Changed. Thanks!
>
> > + //Wait for writers to be done;
> > + try {
> > + writerSignal.await();
>
> The comment makes me think the latch should be named `writersDone`.
Right.
>
> > + } catch (InterruptedException e) {
> > + e.printStackTrace();
>
> Can you make this fail the test?
Yep.
>
> > + }
> > 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?
Ah you are right, the functionality of the test was lost in all the changes. I have made changes so that this test actually causes the read to be attempted after the write, while the write still has the lock.
>
> > + //Let people know reading is done
>
> There's nothing that does what the comment says :(
Right. It was removed with the addition of the CountDownLatches :( Sorry about that.
>
> > +++ 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.
Yes.
>
> > + //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.
Done. It's been placed in a class called CacheTestUtils.
Thanks!!
>
> Thanks,
> Omair
>
> --
> PGP Key: 66484681 (http://pgp.mit.edu/)
> Fingerprint = F072 555B 0A17 3957 4E95 0056 F286 F14F 6648 4681
>
--
Jie Kang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-cachelock-deadlock-6.patch
Type: text/x-patch
Size: 18942 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140926/b892be4e/itw-cachelock-deadlock-6-0001.patch>
More information about the distro-pkg-dev
mailing list