[RFC] Fix IndexOutOfBoundException because of corrupted entry in recently_used file
Jiri Vanek
jvanek at redhat.com
Thu Apr 5 05:59:46 PDT 2012
On 04/04/2012 04:57 PM, Thomas Meyer wrote:
> Am Dienstag, den 03.04.2012, 16:50 +0200 schrieb Jiri Vanek:
>> > Thanx for test! Logic itself is good, but there are two mistakes which causes the test to be not-working.
>> > See notes inline for them.
> Hi,
>
> I hacked a bit on this today. please have a look at the attached patch.
>
> I moved all the format checks in the CacheLRUWrapper.load() method. that
> is the place were I think they actually belong.
>
> I guess these changes will break the CacheReproducerTest as no more
> LRUExceptions are thrown.
I would like to notice outside world that cache was corrupted. (LruCacheException)
ex.printStacktrace can be as good as anything else (and is backward compatible with existing approach.
However to print stacktrace just in debug mode will be probably better idea.
Plus information text I have dared to add.
>
> what do you think about this approach? does this make sense for you?
Yap! I till it excellent. It is also handling multiple launches upon broken cache better then my
original fix did.
Although there was an bug in removing item from cache with running iteration (which was causing
ConcurrentModificationException)
I have dared to fix this and LruCacheException printing and to include working tests to your
original patch. This "improved" one is attached.
However still several questions to your original one in-line:
>
> with kind regards
> thomas
>
>
>
>
> check-cache-data.patch
>
>
> diff -r 093896b370d3 Makefile.am
> --- a/Makefile.am Wed Mar 28 12:08:10 2012 +0200
> +++ b/Makefile.am Wed Apr 04 16:48:09 2012 +0200
> @@ -565,7 +565,7 @@
> cd $(JNLP_TESTS_ENGINE_DIR) ; \
> class_names=`cat $(REPRODUCERS_CLASS_NAMES)` ; \
> CLASSPATH=$(NETX_DIR)/lib/classes.jar:$(JUNIT_JAR):$(JUNIT_RUNNER_JAR):. \
> - $(BOOT_DIR)/bin/java -Dtest.server.dir=$(JNLP_TESTS_SERVER_DEPLOYDIR) -Djavaws.build.bin=$(DESTDIR)$(bindir)/javaws \
> + $(BOOT_DIR)/bin/java -Dtest.server.dir=$(JNLP_TESTS_SERVER_DEPLOYDIR) -Djavaws.build.bin=$(DESTDIR)$(bindir)/$(javaws) \
> -Xbootclasspath:$(RUNTIME) CommandLine $$class_names \
> > stdout.log 2> stderr.log ; \
> cat stdout.log ; \
> diff -r 093896b370d3 netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java
> --- a/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Wed Mar 28 12:08:10 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/cache/CacheLRUWrapper.java Wed Apr 04 16:48:09 2012 +0200
> @@ -107,6 +107,49 @@
> */
> public synchronized void load() {
> cacheOrder.load();
> +
> + /*
> + * clean up possibly corrupted entries
> + */
It is interesting to see how many times is checkData called during single jar run.. I counted
10executions.... But fixing this is definitely work for different patch.
> + if(checkData())
> + store();
> +
> + }
> +
> + /**
> + * check content of cacheOrder and remove invalid/corrupt entries
> + */
> + private boolean checkData () {
> + boolean modified = false;
> +
> + for (Entry<Object, Object> currentEntry : cacheOrder.entrySet()) {
> + final String key = (String) currentEntry.getKey();
> + final String path = (String) currentEntry.getValue();
> +
> + // 1. check key format: "milliseconds,number"
> + try {
> + String sa[] = key.split(",");
> + Long l1 = Long.parseLong(sa[0]);
> + Long l2 = Long.parseLong(sa[1]);
> + } catch (Exception ex) {
> + cacheOrder.remove(key);
> + modified = true;
> + continue;
> + }
> +
> + // 2. check path format - does the path look correct?
> + if (path != null) {
> + if (path.indexOf(cacheDir)< 0) {
> + cacheOrder.remove(key);
> + modified = true;
> + }
> + } else {
> + cacheOrder.remove(key);
> + modified = true;
> + }
> + }
> +
> + return modified;
> }
Really elegant solution this check :)
>
> /**
> @@ -174,15 +217,11 @@
> Collections.sort(entries, new Comparator<Entry<String, String>>() {
> @Override
> public int compare(Entry<String, String> e1, Entry<String, String> e2) {
> - try {
> - Long t1 = Long.parseLong(e1.getKey().split(",")[0]);
> - Long t2 = Long.parseLong(e2.getKey().split(",")[0]);
> + Long t1 = Long.parseLong(e1.getKey().split(",")[0]);
> + Long t2 = Long.parseLong(e2.getKey().split(",")[0]);
>
> - int c = t1.compareTo(t2);
> - return c< 0 ? 1 : (c> 0 ? -1 : 0);
> - } catch (Exception e) {
> - throw new LruCacheException(R("Corrupt LRU file entries"));
> - }
> + int c = t1.compareTo(t2);
> + return c< 0 ? 1 : (c> 0 ? -1 : 0);
> }
> });
> return entries;
> diff -r 093896b370d3 netx/net/sourceforge/jnlp/cache/CacheUtil.java
> --- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java Wed Mar 28 12:08:10 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java Wed Apr 04 16:48:09 2012 +0200
> @@ -308,7 +308,6 @@
> if (cacheFile == null) { // We did not find a copy of it.
> cacheFile = makeNewCacheFile(source, version);
> }
> - lruHandler.store();
This scares me - why it have begin to be removed?
> lruHandler.unlock();
> }
> return cacheFile;
> @@ -324,47 +323,19 @@
> synchronized (lruHandler) {
> File cacheFile = null;
> 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(R("CFakeCache"));
> - lruHandler.clearLRUSortedEntries();
> - lruHandler.store();
> - System.out.println(R("CFakedCache"));
> - } else if (tries == 2) {
> - ex.printStackTrace();
> - System.out.println(R("CStillCorupted"));
> - boolean clearingresult = CacheUtil.clearCache();
> - if (!clearingresult) {
> - throw new InternalError(R("CCleaningUnsuccessful"));
> - }
> - System.out.println(R("CClearedReloading"));
> - lruHandler.clearLRUSortedEntries();
> - lruHandler.store();
> - System.out.println(R("CReloadRestarting"));
Properties texts should be removed to. I have dared to do this. and to reuse little bit modified two
of them.
> + List<Entry<String, String>> entries = lruHandler.getLRUSortedEntries();
>
> - } else {
> - throw new InternalError(R("CStillBroken"));
> - }
> -
> - }
> - } while (entries == null);
> // Start searching from the most recent to least recent.
> for (Entry<String, String> e : entries) {
> final String key = e.getKey();
> final String path = e.getValue();
>
> - if (path != null) {
> - if (pathToURLPath(path).equals(urlPath.getPath())) { // Match found.
> - cacheFile = new File(path);
> - lruHandler.updateEntry(key);
> - break; // Stop searching since we got newest one already.
> - }
> + assert(path!=null);
I'm not friend of assert(...)....
I think this is even not necessary - the self-explaining NullPointerException will be thrown little
bit later. However I do not insists and do as you wish.
> +
> + if (pathToURLPath(path).equals(urlPath.getPath())) { // Match found.
> + cacheFile = new File(path);
> + lruHandler.updateEntry(key);
> + break; // Stop searching since we got newest one already.
> }
> }
> return cacheFile;
> @@ -561,10 +532,13 @@
> * This will remove all old cache items.
> */
> public static void cleanCache() {
> +
> if (okToClearCache()) {
> // First we want to figure out which stuff we need to delete.
> HashSet<String> keep = new HashSet<String>();
> HashSet<String> remove = new HashSet<String>();
> +
> + // FIXME: lock on recently_used file needed?
> lruHandler.load();
hmmm .. I do not know:(
>
> long maxSize = -1; // Default
> @@ -579,57 +553,57 @@
> for (Entry<String, String> e : lruHandler.getLRUSortedEntries()) {
> // Check if the item is contained in cacheOrder.
> final String key = e.getKey();
> - final String value = e.getValue();
> + final String path = e.getValue();
>
> - if (value != null) {
> - File file = new File(value);
> - PropertiesFile pf = new PropertiesFile(new File(value + ".info"));
> - boolean delete = Boolean.parseBoolean(pf.getProperty("delete"));
> + assert(path!=null);
same assert as above...?...
>
> - /*
> - * This will get me the root directory specific to this cache item.
> - * Example:
> - * cacheDir = /home/user1/.icedtea/cache
> - * file.getPath() = /home/user1/.icedtea/cache/0/http/www.example.com/subdir/a.jar
> - * rStr first becomes: /0/http/www.example.com/subdir/a.jar
> - * then rstr becomes: /home/user1/.icedtea/cache/0
> - */
> - String rStr = file.getPath().substring(cacheDir.length());
> - rStr = cacheDir + rStr.substring(0, rStr.indexOf(File.separatorChar, 1));
> - long len = file.length();
> + File file = new File(path);
> + PropertiesFile pf = new PropertiesFile(new File(path + ".info"));
> + boolean delete = Boolean.parseBoolean(pf.getProperty("delete"));
>
> - if (keep.contains(file.getPath().substring(rStr.length()))) {
> - lruHandler.removeEntry(key);
> - continue;
> - }
> -
> - /*
> - * we remove entries from our lru if any of the following condition is met.
> - * Conditions:
> - * - delete: file has been marked for deletion.
> - * - !file.isFile(): if someone tampered with the directory, file doesn't exist.
> - * - maxSize>= 0&& curSize + len> maxSize: If a limit was set and the new size
> - * on disk would exceed the maximum size.
> - */
> - if (delete || !file.isFile() || (maxSize>= 0&& curSize + len> maxSize)) {
> - lruHandler.removeEntry(key);
> - remove.add(rStr);
> - } else {
> - curSize += len;
> - keep.add(file.getPath().substring(rStr.length()));
> + /*
> + * This will get me the root directory specific to this cache item.
> + * Example:
> + * cacheDir = /home/user1/.icedtea/cache
> + * file.getPath() = /home/user1/.icedtea/cache/0/http/www.example.com/subdir/a.jar
> + * rStr first becomes: /0/http/www.example.com/subdir/a.jar
> + * then rstr becomes: /home/user1/.icedtea/cache/0
> + */
> + String rStr = file.getPath().substring(cacheDir.length());
> + rStr = cacheDir + rStr.substring(0, rStr.indexOf(File.separatorChar, 1));
> + long len = file.length();
>
> - for (File f : file.getParentFile().listFiles()) {
> - if (!(f.equals(file) || f.equals(pf.getStoreFile()))){
> - try {
> - FileUtils.recursiveDelete(f, f);
> - } catch (IOException e1) {
> - e1.printStackTrace();
> - }
> - }
> + if (keep.contains(file.getPath().substring(rStr.length()))) {
> + lruHandler.removeEntry(key);
> + continue;
> + }
> +
> + /*
> + * we remove entries from our lru if any of the following condition is met.
> + * Conditions:
> + * - delete: file has been marked for deletion.
> + * - !file.isFile(): if someone tampered with the directory, file doesn't exist.
> + * - maxSize>= 0&& curSize + len> maxSize: If a limit was set and the new size
> + * on disk would exceed the maximum size.
> + */
> + if (delete || !file.isFile() || (maxSize>= 0&& curSize + len> maxSize)) {
> + lruHandler.removeEntry(key);
> + remove.add(rStr);
> + continue;
> + }
> +
> + curSize += len;
> + keep.add(file.getPath().substring(rStr.length()));
> +
> + for (File f : file.getParentFile().listFiles()) {
> + if (!(f.equals(file) || f.equals(pf.getStoreFile()))){
> + try {
> + FileUtils.recursiveDelete(f, f);
> + } catch (IOException e1) {
> + e1.printStackTrace();
> }
> }
> - } else {
> - lruHandler.removeEntry(key);
> +
> }
I must admit that I did not got fully the meaning of this change. But it is working pretty fine and
looks ok :)
> }
> lruHandler.store();
> diff -r 093896b370d3 netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Wed Mar 28 12:08:10 2012 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java Wed Apr 04 16:48:09 2012 +0200
> @@ -713,6 +713,7 @@
> Runtime.getRuntime().addShutdownHook(new Thread() {
> public void run() {
> markNetxStopped();
> + // no lock on recently_used file needed?
I'm not sure again as above :(
> CacheUtil.cleanCache();
> }
> });
>
I hope my small changes will not insult you, but while I was playing with your changeset they looks
to me like essential ones so I put them in instead of describing.
If you will agree with printing of lrucacheexception stacktrace then this is nearly done.
If you will insists on no stacktreace printing or (what I prefer) on printing it out just in debug
mode, then the tests will need little fixing. (for case with no stacktraces it will need to change
testing conditions, and for stacktace printed in debugmode it will need to run javaws in verbose mode)
Best regards and thanx for patch.
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thomasImprovedCacheFix.diff
Type: text/x-patch
Size: 21141 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120405/b4cf79ac/thomasImprovedCacheFix.diff
More information about the distro-pkg-dev
mailing list