[RFC][icedtea-web] PR833 - icedtea-web is failing when cache is corupted.
Jiri Vanek
jvanek at redhat.com
Tue Jan 24 05:40:59 PST 2012
On 01/10/2012 04:31 AM, Deepak Bhole wrote:
> Hi Jiri,
>
> Sorry for the lateness on this. Haven't been able to get to it till now.
>
> Overall, I think the patch is fine. I do have a few minor nitpicks below
> though:
>
> * Jiri Vanek<jvanek at redhat.com> [2011-12-20 11:15]:
>> On 12/19/2011 04:41 PM, Deepak Bhole wrote:
>>> * Jiri Vanek<jvanek at redhat.com> [2011-12-13 11:36]:
>>>> 2011-12-13 Jiri Vanek<jvanek at redhat.com>
>>>>
>>>> Fix for PR844
>>>> * netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java: (getLRUSortedEntries)
>>>> instead of error throwing own LRU exception. Also catches more then NumberFormatException
>>>> (clearLRUSortedEntries) new method - making soft clearing of cache public
>>>> (clearCache) now return true if cache was cleared, false otherwise (or exception)
>>>> * netx/net/sourceforge/jnlp/cache/CacheUtil.java: (getCacheFileIfExist) does three tires to load cache.
>>>> If ifrst fails, then recently_used file is emptied both in memory and on disc.
>>>> When second attemmpt fails, then LRU cache is forcibly cleared. if clearing fails, then error is thrown.
>>>> If it pass, then one more try to load entries is allowed. When third attempt fails, then error is thrown.
>>>> * /netx/net/sourceforge/jnlp/cache/LruCacheException.java:
>>>> new file, for purpose of catching this particular exception
>>>> * netx/net/sourceforge/jnlp/util/PropertiesFile.java: (store) tries to mkdirs to its path.
>>>> It is better then to fail when no cache directory exists.
>>>> * tests/jnlp_tests/signed/CacheReproducer: new reproducr trying severals way of corupted cache
>>>> on several types of jnlp files. Is signed because of reflection used.
>>>> * tests/jnlp_tests/signed/SimpletestSigned1: signed hello world to be used in CacheReproducer tests.
>>>> * tests/netx/jnlp_testsengine/net/sourceforge/jnlp/ServerAccess.java: timeout for processes doubled,
>>>> as clear cache methods sometimes took more then original allowed.
>>>>
>
> The lines above seem to be longer than 80 chars.
wrapped
>
> <snip>
>
>> /**
>> @@ -322,7 +323,37 @@
>> private static File getCacheFileIfExist(File urlPath) {
>> synchronized (lruHandler) {
>> File cacheFile = null;
>> - List<Entry<String, String>> entries = lruHandler.getLRUSortedEntries();
>> + int tries=0;
>> + List<Entry<String, String>> entries=null;
>> + do{
>> + try{
>> + tries++;
>> + entries = lruHandler.getLRUSortedEntries();
>> + }catch(LruCacheException ex){
>> + if (tries==1){
>> + ex.printStackTrace();
>> + System.out.println("Cache is corupted, will be faked now");
>> + lruHandler.clearLRUSortedEntries();
>> + lruHandler.store();
>> + System.out.println("Faked, continuing. It is strongly recomanded to run javaws -Xclearcache and rerun your application as soon as possible.");
>> + }else if (tries==2){
>> + ex.printStackTrace();
>> + System.out.println("Cache is still corupted, will be cleared now");
>> + boolean clearingresult=CacheUtil.clearCache();
>> + if (!clearingresult){
>> + throw new InternalError("Clearing was not sucessfull, probably due to another javaws instance runnin.. Try to shut down all instances of javaws, run javaws -Xclearcache and rerun yout jnlp file");
>> + }
>> + System.out.println("Cleared, reloading");
>> + lruHandler.clearLRUSortedEntries();
>> + lruHandler.store();
>> + System.out.println("Reload, restarting, it is strongly recomanded to run javaws -Xclearcache and rerun your application as soon as possible.");
>> +
>> + }else{
>> + throw new InternalError("LRU cache was corrupted. Was cleared, but still is corrupted. Try to shut down all instances of javaws, run javaws -Xclearcache and rerun yout jnlp file");
>> + }
>> +
>> + }
>> + }while(entries==null);
>
> There are spacing issues in the above code e.g. no space between "do"
> and "{" .. please follow the convention here:
> http://icedtea.classpath.org/wiki/IcedTea-Web#Code_style
>
> Also, the test in the above messages should use the R function, not the
> direct text itself. Please also check for spelling and grammar errors
> e.g. "and rerun yout jnlp" is incorrect.
ufff... a lot of work done here. Hope that it as it should be now!
>
> Logic wise, the code seems fine to me.
>
>> // Start searching from the most recent to least recent.
>> for (Entry<String, String> e : entries) {
>> final String key = e.getKey();
>> diff -r 71f338e881d2 netx/net/sourceforge/jnlp/cache/LruCacheException.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/netx/net/sourceforge/jnlp/cache/LruCacheException.java Tue Dec 20 17:35:08 2011 +0100
>> @@ -0,0 +1,23 @@
>> +/*
>> + * To change this template, choose Tools | Templates
>> + * and open the template in the editor.
>> + */
>> +
>> +package net.sourceforge.jnlp.cache;
>> +
>> +/**
>> + *
>> + * @author jvanek
>
> Just a minor nitpick, please remove the above :) I know the IDE adds
> them but we don't use it in most files, and most of the files are
> modified by many people over time anyway.
>
> Also, please add the license header.
>
Done, thanx for catch.
> <snip>
>
>> diff -r 71f338e881d2 tests/jnlp_tests/signed/CacheReproducer/resources/CacheReproducer1_1.jnlp
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/jnlp_tests/signed/CacheReproducer/resources/CacheReproducer1_1.jnlp Tue Dec 20 17:35:08 2011 +0100
>> @@ -0,0 +1,16 @@
>> +<?xml version="1.0" encoding="utf-8"?>
>> +<jnlp spec="1.0"
>> + codebase="./"
>> + href="CacheReproducer1_1.jnlp">
>> +<information>
>> +<title>read properties using System.getenv()</title>
>
> Few things:
>
> 1. The title in all of the jnlp's appear to be incorrect -- they should be
> fixed to be more correct.
Done. Thanx for catch!
> 2. Can we change the file naming to more accurately reflect what is
> being tested?
I think no - because the test applications themselves in this case really do nearly nothing - or better, they do as they are Named - (Signed)SimpleTest1 is just (signed) hello world, and CacheReproducer is used in CacheTests. He itself do ..nothing.
>
> <snip>
>
>> + @Test
>> + public void cacheIsWorkingTestSigned() throws Exception {
>> + clearAndEvaluateCache();
>> + evaluateSimpleTest1OkCache(runSimpleTest1Signed());
>> + assertCacheIsNotEmpty();
>> +
>> + }
>> + private class ParalelSimpleTestRunner extends Thread {
>
> Parallel is spelt incorrectly here and in the function below it. Also,
> There also seems to be a missing newline before the function, an extra
> new-line in the function above it, extra space between "extends" and
> "Thread" etc.
>
Done
> <snip>
>
>> + }
>> +
>> + @Test
>> + public void assertBreakersAreWorking() {
>> + String s = "#netx file\n"
>> + + "#Mon Dec 12 16:20:46 CET 2011\n"
>> + + "1323703236508,0=/home/jvanek/.icedtea/cache/0/http/localhost/ReadPropertiesBySignedHack.jnlp\n"
>> + + "1323703243086,2=/home/jvanek/.icedtea/cache/2/http/localhost/ReadProperties.jar\n"
>> + + "1323703243082,1=/home/jvanek/.icedtea/cache/1/http/localhost/ReadPropertiesBySignedHack.jar";
>
> If the hardcoded username on purpose?
There is no need to have any path. The string 1234567890000,10=/jedna/dve/tri/ctyry/ will do the same job, but I wanted to use as close paths as are normally in cache file. I have replaced my name by some random string. It is testing just breaking reg-ex and logic around it.
>
> Lastly, please check code styling overall. I have pointed it out wherever I
> saw it, but just in case I missed it somewhere..
>
> Everything else looks good!
>
> Thanks,
> Deepak
TYVM for review! J.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: LRUcachePatch.diff4
Url: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120124/70579c6c/LRUcachePatch.diff4
More information about the distro-pkg-dev
mailing list