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

[Lukasz Dracz]

Hello,

The patch looks good to me ! 

Only one small nit

         } finally {
-            entry.unlock();
+             entry.unlock();
         }

I think you added an extra space, which is not needed.

Other than that I give it +1 from me
Thanks for the patch.

Regards,
Lukasz Dracz

----- Original Message -----
> From: "Jie Kang" <jkang at redhat.com>
> To: "Lukasz Dracz" <ldracz at redhat.com>
> Cc: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
> Sent: Thursday, February 12, 2015 11:27:03 AM
> Subject: Re: [rfc][icedtea-web] Refactor initializeResources in ResourceDownloader
> 
> 
> 
> ----- 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
>