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

[Jie Kang]

----- Original Message -----
> On 02/11/2015 08:45 PM, Lukasz Dracz wrote:
> > 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.
> >
> > +    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 ?
> >
> > +                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.
> >
> > Other than that the refactor looks good to me :) !
> >
> Thak you for review, both poatch and review rally looks good.
> 
> I have only one solid concern - always you wrote COPYPASTED you meant MOVED,
> right?
> 
> If there is anything really copypasted, please change it to calls to methods.
> 
> 
> Also I have one idea:
> 
> Although the static impl of isConnectable may remain where it is, it would be
> nice to have shortcut
> to it via
> 
> resource.isConnectable() (as url is hiddne in resource)

I like this, I'll add it in as well.


Thanks!

> 
> 
> hmm?
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern