[RFC][icedtea-web] PR833 - icedtea-web is failing when cache is corupted.
Deepak Bhole
dbhole at redhat.com
Mon Jan 9 19:31:47 PST 2012
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.
<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.
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.
<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.
2. Can we change the file naming to more accurately reflect what is
being tested?
<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.
<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?
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
More information about the distro-pkg-dev
mailing list