changeset 1155:62290d68e546

Refactor initializeResources in ResourceDownloader.java 2015-02-17 Jie Kang <jkang@redhat.com> Refactor initializeResources in ResourceDownloader. * netx/net/sourceforge/jnlp/cache/Resource.java: (isConnectable) new method checking if we can connect to the resources URL * netx/net/sourceforge/jnlp/cache/ResourceDownloader.java: (initializeResources), (initializeOnlineResource), (initializeFromURL), (initializeOfflineResource) refactored methods to handle offline and online cases separately * netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java: (isConnectable) new method checking if we can connect to URL argument * tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java: (testDownloadLocalResourceFails): test modified to expect error status on failure instead of uncaught exception
author Jie Kang <jkang@redhat.com>
date Tue, 17 Feb 2015 16:33:14 -0500
parents a8baec8d9d21
children a1b50e850558
files ChangeLog netx/net/sourceforge/jnlp/cache/Resource.java netx/net/sourceforge/jnlp/cache/ResourceDownloader.java netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java
diffstat 5 files changed, 107 insertions(+), 57 deletions(-) [+]
line wrap: on
line diff
--- a/ChangeLog	Fri Feb 13 12:48:24 2015 +0100
+++ b/ChangeLog	Tue Feb 17 16:33:14 2015 -0500
@@ -1,3 +1,18 @@
+2015-02-17  Jie Kang  <jkang@redhat.com>
+
+	Refactor initializeResources in ResourceDownloader.
+	* netx/net/sourceforge/jnlp/cache/Resource.java: (isConnectable) new method
+	checking if we can connect to the resources URL
+	* netx/net/sourceforge/jnlp/cache/ResourceDownloader.java:
+	(initializeResources), (initializeOnlineResource), (initializeFromURL),
+	(initializeOfflineResource) refactored methods to handle offline and online
+	cases separately
+	* netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java: (isConnectable) new
+	method checking if we can connect to URL argument
+	* tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java:
+	(testDownloadLocalResourceFails): test modified to expect error status
+	on failure instead of uncaught exception
+
 2015-02-13  Jiri Vanek  <jvanek@redhat.com>
 
 	Fixed few small errors in jacoco processing after removal of bootstrap.
--- a/netx/net/sourceforge/jnlp/cache/Resource.java	Fri Feb 13 12:48:24 2015 +0100
+++ b/netx/net/sourceforge/jnlp/cache/Resource.java	Tue Feb 17 16:33:14 2015 -0500
@@ -25,6 +25,7 @@
 
 import net.sourceforge.jnlp.DownloadOptions;
 import net.sourceforge.jnlp.Version;
+import net.sourceforge.jnlp.runtime.JNLPRuntime;
 import net.sourceforge.jnlp.util.UrlUtils;
 import net.sourceforge.jnlp.util.WeakList;
 
@@ -425,6 +426,10 @@
         return this.downloadOptions;
     }
 
+    public boolean isConnectable() {
+        return JNLPRuntime.isConnectable(this.location);
+    }
+
     @Override
     public int hashCode() {
         // FIXME: should probably have a better hashcode than this, but considering
--- a/netx/net/sourceforge/jnlp/cache/ResourceDownloader.java	Fri Feb 13 12:48:24 2015 +0100
+++ b/netx/net/sourceforge/jnlp/cache/ResourceDownloader.java	Tue Feb 17 16:33:14 2015 -0500
@@ -110,58 +110,57 @@
         }
     }
 
-    /**
-     * Open a URL connection and get the content length and other
-     * fields.
-     */
     private void initializeResource() {
-        //verify connection
-        if(!JNLPRuntime.isOfflineForced()){
-            JNLPRuntime.detectOnline(resource.getLocation()/*or doenloadLocation*/);
+        if (!JNLPRuntime.isOfflineForced() && resource.isConnectable()) {
+            initializeOnlineResource();
+        } else {
+            initializeOfflineResource();
         }
+    }
 
+    private void initializeOnlineResource() {
+        try {
+            URL finalLocation = findBestUrl(resource);
+            if (finalLocation != null) {
+                initializeFromURL(finalLocation);
+            } else {
+                initializeOfflineResource();
+            }
+        } catch (Exception e) {
+            OutputController.getLogger().log(e);
+            resource.changeStatus(EnumSet.noneOf(Resource.Status.class), EnumSet.of(ERROR));
+            synchronized (lock) {
+                lock.notifyAll(); // wake up wait's to check for completion
+            }
+            resource.fireDownloadEvent(); // fire ERROR
+        }
+    }
+
+    private void initializeFromURL(URL location) throws IOException {
         CacheEntry entry = new CacheEntry(resource.getLocation(), resource.getRequestVersion());
         entry.lock();
+        try {
+            resource.setDownloadLocation(location);
+            URLConnection connection = ConnectionFactory.getConnectionFactory().openConnection(location); // this won't change so should be okay not-synchronized
+            connection.addRequestProperty("Accept-Encoding", "pack200-gzip, gzip");
 
-        try {
             File localFile = CacheUtil.getCacheFile(resource.getLocation(), resource.getDownloadVersion());
-            long size = 0;
-            boolean current = true;
-            //this can be null, as it is always filled in online mode, and never read in offline mode
-            URLConnection connection = null;
-            if (localFile != null) {
-                size = localFile.length();
-            } else if (!JNLPRuntime.isOnline()) {
-                OutputController.getLogger().log(OutputController.Level.ERROR_ALL, "You are trying to get resource " + resource.getLocation().toExternalForm() + " but you are in offline mode, and it is not in cache. Attempting to continue, but you may expect failure");
-            }
-            if (JNLPRuntime.isOnline()) {
-                // connect
-                URL finalLocation = findBestUrl(resource);
+            long size = connection.getContentLengthLong();
 
-                if (finalLocation == null) {
-                    OutputController.getLogger().log(OutputController.Level.ERROR_ALL, "Attempted to download " + resource.getLocation() + ", but failed to connect!");
-                    throw new NullPointerException("finalLocation == null"); // Caught below
-                }
-
-                resource.setDownloadLocation(finalLocation);
-                connection = ConnectionFactory.getConnectionFactory().openConnection(finalLocation); // this won't change so should be okay not-synchronized
-                connection.addRequestProperty("Accept-Encoding", "pack200-gzip, gzip");
-
-                size = connection.getContentLength();
-                current = CacheUtil.isCurrent(resource.getLocation(), resource.getRequestVersion(), connection.getLastModified()) && resource.getUpdatePolicy() != UpdatePolicy.FORCE;
-                if (!current) {
-                    if (entry.isCached()) {
-                        entry.markForDelete();
-                        entry.store();
-                        // Old entry will still exist. (but removed at cleanup)
-                        localFile = CacheUtil.makeNewCacheFile(resource.getLocation(), resource.getDownloadVersion());
-                        CacheEntry newEntry = new CacheEntry(resource.getLocation(), resource.getRequestVersion());
-                        newEntry.lock();
-                        entry.unlock();
-                        entry = newEntry;
-                    }
+            boolean current = CacheUtil.isCurrent(resource.getLocation(), resource.getRequestVersion(), connection.getLastModified()) && resource.getUpdatePolicy() != UpdatePolicy.FORCE;
+            if (!current) {
+                if (entry.isCached()) {
+                    entry.markForDelete();
+                    entry.store();
+                    // Old entry will still exist. (but removed at cleanup)
+                    localFile = CacheUtil.makeNewCacheFile(resource.getLocation(), resource.getDownloadVersion());
+                    CacheEntry newEntry = new CacheEntry(resource.getLocation(), resource.getRequestVersion());
+                    newEntry.lock();
+                    entry.unlock();
+                    entry = newEntry;
                 }
             }
+
             synchronized (resource) {
                 resource.setLocalFile(localFile);
                 // resource.connection = connection;
@@ -174,7 +173,7 @@
             }
 
             // update cache entry
-            if (!current && JNLPRuntime.isOnline()) {
+            if (!current) {
                 entry.setRemoteContentLength(connection.getContentLengthLong());
                 entry.setLastModified(connection.getLastModified());
             }
@@ -189,16 +188,40 @@
 
             // explicitly close the URLConnection.
             ConnectionFactory.getConnectionFactory().disconnect(connection);
-        } catch (Exception ex) {
-            OutputController.getLogger().log(ex);
-            resource.changeStatus(EnumSet.noneOf(Resource.Status.class), EnumSet.of(ERROR));
+        } finally {
+            entry.unlock();
+        }
+    }
+
+    private void initializeOfflineResource() {
+        CacheEntry entry = new CacheEntry(resource.getLocation(), resource.getRequestVersion());
+        entry.lock();
+
+        try {
+            File localFile = CacheUtil.getCacheFile(resource.getLocation(), resource.getDownloadVersion());
+
+            if (localFile != null && localFile.exists()) {
+                long size = localFile.length();
+
+                synchronized (resource) {
+                    resource.setLocalFile(localFile);
+                    resource.setSize(size);
+                    resource.changeStatus(EnumSet.of(PREDOWNLOAD, DOWNLOADING), EnumSet.of(DOWNLOADED));
+                }
+            } else {
+                OutputController.getLogger().log(OutputController.Level.ERROR_ALL, "You are trying to get resource " + resource.getLocation().toExternalForm() + " but it is not in cache and could not be downloaded. Attempting to continue, but you may expect failure");
+                resource.changeStatus(EnumSet.noneOf(Resource.Status.class), EnumSet.of(ERROR));
+            }
+
             synchronized (lock) {
                 lock.notifyAll(); // wake up wait's to check for completion
             }
-            resource.fireDownloadEvent(); // fire ERROR
+            resource.fireDownloadEvent(); // fire CONNECTED or ERROR
+
         } finally {
             entry.unlock();
         }
+
     }
 
     /**
--- a/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java	Fri Feb 13 12:48:24 2015 +0100
+++ b/netx/net/sourceforge/jnlp/runtime/JNLPRuntime.java	Tue Feb 17 16:33:14 2015 -0500
@@ -416,18 +416,23 @@
         if (onlineDetected != null) {
             return;
         }
+
+        JNLPRuntime.setOnlineDetected(isConnectable(location));
+    }
+
+    public static boolean isConnectable(URL location) {
+        if (location.getProtocol().equals("file")) {
+            return true;
+        }
+
         try {
-            if (location.getProtocol().equals("file")) {
-                return;
-            }
-            //Checks the offline/online status of the system.
             InetAddress.getByName(location.getHost());
-        } catch (UnknownHostException ue) {
-            OutputController.getLogger().log(OutputController.Level.ERROR_ALL, "The host of " + location.toExternalForm() + " file should be located seems down, or you are simply offline.");
-            JNLPRuntime.setOnlineDetected(false);
-            return;
+        } catch (UnknownHostException e) {
+            OutputController.getLogger().log(OutputController.Level.ERROR_ALL, "The host of " + location.toExternalForm() + " file seems down, or you are simply offline.");
+            return false;
         }
-        setOnlineDetected(true);
+
+        return true;
     }
    
     /**
--- a/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java	Fri Feb 13 12:48:24 2015 +0100
+++ b/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceDownloaderTest.java	Tue Feb 17 16:33:14 2015 -0500
@@ -436,7 +436,7 @@
         assertEquals(expected, actual);
     }
 
-    @Test (expected = IllegalArgumentException.class)
+    @Test
     public void testDownloadLocalResourceFails() throws IOException {
         String expected = "local-resource";
         File localFile = Files.createTempFile("download-local", ".temp").toFile();
@@ -453,6 +453,8 @@
 
         resource.setStatusFlag(Resource.Status.PRECONNECT);
         resourceDownloader.run();
+
+        assertTrue(resource.hasFlags(EnumSet.of(Resource.Status.ERROR)));
     }
 
     @Test