[rfc][icedtea-web] Refactor initializeResources in ResourceDownloader

[Jie Kang]

----- Original Message -----
> Hello,
> 
> > Hello,
> > 
> > 
> > After the lengthy discussion in the thread for "[rfc][icedtea-web] Check
> > online detection even if checked before", I present another solution.
> > 
> > The initializeResources method is refactored to incorporate online/offline
> > more explicitly and the JNLPRuntime has a new method 'isConnectable(URL)'
> > that is used to determine whether or not a connection can be made to the
> > host of a supplied URL. The previous functionality of IT-W's online/offline
> > system is maintained with no functional changes to the other methods in
> > JNLPRuntime.
> > 
> > Important bits:
> > 
> >     private void initializeResource() {
> >         if (!JNLPRuntime.isOfflineForced() &&
> >         JNLPRuntime.isConnectable(resource.getLocation())) {
> >             initializeOnlineResource();
> >         } else {
> >             initializeOfflineResource();
> >         }
> >     }
> > 
> > This new initial method makes it very clear what's going to happen. Rather
> > than using JNLPRuntime.detectOnline && JNLPRuntime.isOnline, we now use the
> > new JNLPRuntime.isConnectable. The code within these methods is basically
> > the same as the code within the old initializeResource, just copy-pasted
> > into appropriate section.
> > 
> >     public static void detectOnline(URL location) {
> >         if (onlineDetected != null) {
> >             return;
> >         }
> > 
> >         JNLPRuntime.setOnlineDetected(isConnectable(location));
> >     }
> > 
> >     public static boolean isConnectable(URL location) {
> >         if (location.getProtocol().equals("file")) {
> >             return true;
> >         }
> > 
> >         try {
> >             OutputController.getLogger().log(OutputController.Level.ERROR_ALL,
> >             "The host of " + location.toExternalForm() + " file should be
> >             located seems down, or you are simply offline.");
> >             InetAddress.getByName(location.getHost());
> >         } catch (UnknownHostException e) {
> >             return false;
> >         }
> > 
> >         return true;
> >     }
> > 
> > Note that detectOnline has been refactored to call isConnectable. The
> > functionality is the same though as 'isConnectable' is copy-pasted code
> > from
> > the original detectOnline.
> > 
> > 
> > So end-result is that the original framework still exists and is the same.
> > The part that was broken for ResourceTracker has been fixed using the
> > refactored 'initializeResource' along with the 'isConnectable' method.
> > 
> > Thoughts?
> 
> Looks good,
> 
> A few nits,
> 
> diff --git a/netx-dist-tests-whitelist b/netx-dist-tests-whitelist
> --- a/netx-dist-tests-whitelist
> +++ b/netx-dist-tests-whitelist
> @@ -1,1 +1,1 @@
> -.*
> +JSToJSet.*
> 
> I don't think you meant this to be changed.

Right. Removed from changeset.

> 
> +    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.connection = connection;
> +                    resource.setSize(size);
> +                    resource.changeStatus(EnumSet.of(PREDOWNLOAD,
> DOWNLOADING), EnumSet.of(DOWNLOADED));
> 
> Since this is a refactor can we get rid of // resource.connection =
> connection ? or is there a reason to keep this ?

Removed. Thanks for that.

> 
> +                synchronized (lock) {
> +                    lock.notifyAll(); // wake up wait's to check for
> completion
> +                }
> +                resource.fireDownloadEvent(); // fire CONNECTED
> +
> +            } 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
> +            }
> 
> synchronized(lock) ... to .. resource.fireDownloadEvent() is duplicated in
> the if and else, Could you move that out of the if statement to reduce the
> code duplication.

Right. The code has been moved out.



Along with these two, I've introduced a new method, boolean Resource.isConnectable(URL) that funnels into JNLPRuntime's isConnectable. This is along the lines of jvanek's suggestion.



How does it look now?


Regards,

> 
> Other than that the refactor looks good to me :) !
> 
> Thank you,
> Lukasz Dracz
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern
-------------- next part --------------
A non-text attachment was scrubbed...
Name: itw-res-init-refactor-2.patch
Type: text/x-patch
Size: 10687 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150212/6947dcfb/itw-res-init-refactor-2.patch>