[icedtea-web] RFC: Fix caching of compressed content

[Omair Majid]

Hi,

It was brought to my attention that icedtea-web was re-downloading the
jars (which should have been cached) on subsequent runs of a jnlp
application. I dug into it and found a real bug.

To decide if an item is cached, icedtea-web does a HEAD request and
compares the Content-Length with the size of the file. This works fine
when the jars were not compressed. When the jars are fetched compressed
and then uncompressed on disk, there is a mismatch between the sizes and
icedtea-web decides the content is not cached.

The attached patch stores the compressed size in the cache info file and
uses that instead of the actual file size in the size-comparison.

Thoughts?

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681
-------------- next part --------------
diff --git a/ChangeLog b/ChangeLog
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2014-05-02  Omair Majid  <omajid at redhat.com>
+
+	* netx/net/sourceforge/jnlp/cache/CacheEntry.java: Add
+	KEY_CONTENT_ORIGINAL_LENGTH.
+	(getOriginalContentLength, setOriginalContentLength)
+	(getLongKey(String,long)): New methods.
+	(isCached): Check if the original content length is recorded and use it,
+	if available, as the content length.
+	* netx/net/sourceforge/jnlp/cache/ResourceTracker.java (downloadResource):
+	If the content was compressed, store original content length in the cache
+	entry.
+	* tests/netx/unit/net/sourceforge/jnlp/cache/CacheEntryTest.java
+	(testOriginalContentLengthIsSetCorrectly): New method.
+
 2014-05-02  Omair Majid  <omajid at redhat.com>
 
 	* netx/net/sourceforge/jnlp/cache/CacheEntry.java: Use
diff --git a/netx/net/sourceforge/jnlp/cache/CacheEntry.java b/netx/net/sourceforge/jnlp/cache/CacheEntry.java
--- a/netx/net/sourceforge/jnlp/cache/CacheEntry.java
+++ b/netx/net/sourceforge/jnlp/cache/CacheEntry.java
@@ -34,6 +34,7 @@
 public class CacheEntry {
 
     private static final String KEY_CONTENT_LENGTH = "content-length";
+    private static final String KEY_CONTENT_ORIGINAL_LENGTH = "content-original-length";
     private static final String KEY_LAST_MODIFIED = "last-modified";
     private static final String KEY_LAST_UPDATED = "last-updated";
 
@@ -94,6 +95,24 @@
         setLongKey(KEY_CONTENT_LENGTH, length);
     }
 
+    /**
+     * Return the length of the original content that was cached. May be different
+     * from the actual file size due to (de)compression.
+     *
+     * @return the content length or -1 if unknown.
+     */
+    public long getOriginalContentLength() {
+        return getLongKey(KEY_CONTENT_ORIGINAL_LENGTH, -1);
+    }
+
+    /**
+     * Set the length of the original content that was cached. May be different
+     * from the actual file size due to (de)compression.
+     */
+    public void setOriginalContentLength(long contentLength) {
+        setLongKey(KEY_CONTENT_ORIGINAL_LENGTH, contentLength);
+    }
+
     public long getLastModified() {
         return getLongKey(KEY_LAST_MODIFIED);
     }
@@ -103,10 +122,15 @@
     }
 
     private long getLongKey(String key) {
+        return getLongKey(key, 0);
+    }
+
+    private long getLongKey(String key, long defaultValue) {
         try {
             return Long.parseLong(properties.getProperty(key));
         } catch (Exception ex) {
-            return 0;
+            OutputController.getLogger().log(ex);
+            return defaultValue;
         }
     }
 
@@ -156,6 +180,11 @@
 
         try {
             long cachedLength = localFile.length();
+            String originalLength = properties.getProperty(KEY_CONTENT_ORIGINAL_LENGTH);
+            if (originalLength != null) {
+                cachedLength = Long.parseLong(originalLength);
+            }
+
             long remoteLength = Long.parseLong(properties.getProperty(KEY_CONTENT_LENGTH, "-1"));
 
             if (remoteLength >= 0 && cachedLength != remoteLength)
diff --git a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
--- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
+++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java
@@ -757,6 +757,9 @@
             }
 
             if (!downloadLocationFile.getPath().equals(finalFile.getPath())) {
+                origEntry.setOriginalContentLength(downloadEntry.getContentLength());
+                origEntry.store();
+
                 downloadEntry.markForDelete();
                 downloadEntry.store();
             }
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
@@ -94,4 +94,14 @@
         assertEquals(CONTENT_LENGTH, entry.getContentLength());
     }
 
+    @Test
+    public void testOriginalContentLengthIsSetCorrectly() {
+        long ORIGINAL_CONTENT_LENGTH = 1000;
+
+        CacheEntry entry = new CacheEntry(url, version);
+        entry.setOriginalContentLength(ORIGINAL_CONTENT_LENGTH);
+
+        assertEquals(ORIGINAL_CONTENT_LENGTH, entry.getOriginalContentLength());
+    }
+
 }