[RFC][icedtea-web]: DownloadService implementation
Adam Domurad
adomurad at redhat.com
Fri Sep 28 11:36:46 PDT 2012
On Mon, 2012-09-24 at 13:58 -0400, Saad Mohammad wrote:
> Hello,
>
> I found a little bug while writing the reproducer for this implementation. I
> have fixed this issue and attached the updated patch with a Changelog entry
> (sorry, I somehow missed adding the Changelog entry in the previous email).
>
> Thanks.
>
> Changelog:
>
> 2012-09-07 Saad Mohammad <smohammad at redhat.com>
>
> Core implementation of DownloadService.
> * netx/net/sourceforge/jnlp/cache/CacheUtil.java (getCacheParentDirectory):
> Returns the parent directory of the cached resource.
> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
> (addNewJar): Adds a new jar to the classloader with specified
> UpdatePolicy.
> (getLoaderByJnlpUrl): Returns the classloader of the jnlp file
> specified.
> (getLoaderByResourceUrl): Returns the classloader that contains the
> specified jar.
> (getJars): Returns jars from the JNLP file with the specified
> partname.
> (removeCachedJars): Removes jar from cache.
> (removeJars): Help removeCachedJars() remove jars from the
> filesystem.
> (downloadJars): Downloads jars identified by part name.
> (initializeNewJarDownload): Downloads and initializes jars into the
> current loader.
> (manageExternalJars): Manages jars that are not mentioned in the
> JNLP file.
> (loadExternalResouceToCache): Used by DownloadService to download
> and initalize resources that are not mentioned in the jnlp file.
> (removeExternalCachedResource): Used by DownloadService to remove
> resources from cache that are not mentioned in the jnlp file.
> (isExternalResourceCached): Determines if the resource that is not
> mentioned in the jnlp file is cached and returns a boolean with the
> result.
> * netx/net/sourceforge/jnlp/services/XDownloadService.java:
> Core implementation of DownloadService.
>
>
Thanks for the implementation ! Comments inline.
> diff --git a/netx/net/sourceforge/jnlp/cache/CacheUtil.java b/netx/net/sourceforge/jnlp/cache/CacheUtil.java
> --- a/netx/net/sourceforge/jnlp/cache/CacheUtil.java
> +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java
> @@ -367,6 +367,17 @@
> }
>
> /**
> + * Returns the parent directory of the cached resource.
> + * @param path The path of the cached resource directory.
> + */
> + public static String getCacheParentDirectory(String path) {
Less fragile is:
return new File(path).getParent();
File.separatorChar will cause problems with Unix-style paths on Windows :)
(Yes... *someone* might run this on Windows.)
> + int len = cacheDir.length();
> + int index = path.indexOf(File.separatorChar, len + 1);
> + String test = path.substring(0, index);
> + return test + "/";
> + }
> +
> + /**
> * This will create a new entry for the cache item. It is however not
> * initialized but any future calls to getCacheFile with the source and
> * version given to here, will cause it to return this item.
> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
As we discussed on IRC, this class is already > 2250 lines! Any more
and it'll surely collapse into a black hole. :) I'm guilty of hacking
on lines to this class too, but it really needs to shrink. A bunch of this new
logic should be part of its own class. JNLPClassLoader can stand to
benefit from the 'one class, one purpose' idea.
The question then remains where the logic should go. I'm guessing the
add/removeJars must stay in JNLPClassLoader, but not much else. I think
the cleanest solution is to have a class with static methods that
operates on JNLPClassLoader. The public interface AFAICS is:
public void loadExternalResouceToCache(URL ref, String version)
public void removeExternalCachedResource(URL ref, String version)
public boolean isExternalResourceCached(URL ref, String version)
So I'm thinking the rest of the methods would be private static within
this class, all take a JNLPClassLoader as their first parameter. I
guess a good name for this would be 'JNLPExternalCachedResources'.
This, at least to me, sounds like a class full of static method
(somewhat like "Arrays"). But, name it whatever seems natural to you.
> @@ -104,6 +104,13 @@
> final public static String TEMPLATE = "JNLP-INF/APPLICATION_TEMPLATE.JNLP";
> final public static String APPLICATION = "JNLP-INF/APPLICATION.JNLP";
>
> + /** Actions to specify how cache is to be managed **/
> + static class DownloadAction{
Absolute nit here, but I like a space before {, ie "static class DownloadAction {".
More concrete though, this should definitely be an enum.
> + final static int DOWNLOADTOCACHE = 1;
Underscores separating all-caps constants seems to be a bit more
consistent with the rest of ITW. (alt+shift+r will rename things it in
Eclipse in a pinch, in case you aren't familiar with/forget the shortcut)
>+ final static int REMOVEFROMCACHE = 2;
> + final static int CHECKCACHE = 3;
> + }
> +
> /** True if the application has a signed JNLP File */
> private boolean isSignedJNLP = false;
>
> @@ -1622,13 +1629,22 @@
> * @param desc the JARDesc for the new jar
> */
> private void addNewJar(final JARDesc desc) {
> + this.addNewJar(desc, JNLPRuntime.getDefaultUpdatePolicy());
> + }
> +
> + /**
> + * Adds a new JARDesc into this classloader.
> + * @param desc the JARDesc for the new jar
> + * @param updatePolicy the UpdatePolicy for the resource
> + */
> + private void addNewJar(final JARDesc desc, UpdatePolicy updatePolicy) {
>
> available.add(desc);
>
> tracker.addResource(desc.getLocation(),
> desc.getVersion(),
> null,
> - JNLPRuntime.getDefaultUpdatePolicy()
> + updatePolicy
> );
>
> // Give read permissions to the cached jar file
> @@ -2112,6 +2128,253 @@
> }
>
> /**
> + * Locates the JNLPClassLoader of the JNLP file.
> + *
> + * @param urlToJnlpFile Path of the JNLP file. If null, main JNLP's file location
> + * be used instead
> + * @return the JNLPClassLoader of the JNLP file.
> + */
> + protected JNLPClassLoader getLoaderByJnlpUrl(URL urlToJnlpFile) {
> +
> + if (urlToJnlpFile == null)
> + urlToJnlpFile = file.getFileLocation();
> +
> + for (int i = 0; i < loaders.length; i++) {
> + if (file.getFileLocation().equals(urlToJnlpFile)) {
> + return this;
> + }
This can be out of the loop.
> + else if (i < loaders.length - 1) {
> + JNLPClassLoader foundLoader = loaders[i + 1].getLoaderByJnlpUrl(urlToJnlpFile);
> +
> + if (foundLoader != null)
> + return foundLoader;
> + }
> + }
This logic is a little hard to follow, wouldn't this rephrasing of the loop:
if (file.getFileLocation().equals(urlToJnlpFile)) {
return this;
}
for (JNLPClassLoader loader : loaders) {
if (loader != this) {
JNLPClassLoader foundLoader = loader.getLoaderByJnlpUrl(urlToJnlpFile);
if (foundLoader != null) {
return foundLoader;
}
}
}
Does the same thing (I believe?) + its more explicit than relying on [0] == this.
Logic like this probably could live outside of the class itself.
> +
> + return null;
> + }
> +
> + /**
> + * Locates the JNLPClassLoader of the JNLP file's resource.
> + *
> + * @param urlToJnlpFile Path of the launch or extension JNLP File. If null,
> + * main JNLP's file location will be used instead.
> + * @param version The version of resource. Is null if no version is specified
> + * @return the JNLPClassLoader of the JNLP file's resource.
> + */
> + protected JNLPClassLoader getLoaderByResourceUrl(URL ref, String version) {
> + Version resourceVersion = (version == null) ? null : new Version(version);
> +
> + for (int i = 0; i < loaders.length; i++) {
> + ResourcesDesc resources = loaders[i].getJNLPFile().getResources();
> +
> + for (JARDesc eachJar : resources.getJARs()) {
> + if (ref.equals(eachJar.getLocation()) &&
> + (resourceVersion == null || resourceVersion.equals(eachJar.getVersion()))) {
> + return this;
> + }
> + }
> +
> + if (i < loaders.length - 1) {
> + JNLPClassLoader foundLoader = loaders[i + 1].getLoaderByResourceUrl(ref, version);
> +
> + if (foundLoader != null)
> + return foundLoader;
> + }
> + }
> + return null;
> + }
I would prefer this as two loops:
for (JNLPClassLoader loader : loaders) {
ResourcesDesc resources = loaders[i].getJNLPFile().getResources();
for (JARDesc eachJar : resources.getJARs()) {
if (ref.equals(eachJar.getLocation()) &&
(resourceVersion == null || resourceVersion.equals(eachJar.getVersion()))) {
return this;
}
}
}
for (JNLPClassLoader loader : loaders) {
if (loader != this) {
JNLPClassLoader foundLoader = loader.getLoaderByResourceUrl(ref, version);
if (foundLoader != null) {
return foundLoader;
}
}
}
Assuming I understand the intent, anyhow.
> +
> + /**
> + * Returns jars from the JNLP file with the part name provided.
> + *
> + * @param ref Path of the launch or extension JNLP File containing the
> + * resource. If null, main JNLP's file location will be used instead.
> + * @param part The name of the part.
> + * @return jars found.
> + */
> + public JARDesc[] getJars(URL ref, String part, Version version) {
> + JNLPClassLoader foundLoader = this.getLoaderByJnlpUrl(ref);
> +
> + if (foundLoader != null) {
> + ArrayList<JARDesc> foundJars = new ArrayList<JARDesc>();
Nit, but as a rule of thumb try and use the minimal interface needed as the declared type.
So this would be List<JARDesc> foundJars = new ArrayList<JARDesc>();.
This is probably a matter of taste in this case though, so proceed as you will.
> +
> + for (JARDesc eachJar : foundLoader.getJNLPFile().getResources().getJARs(part)) {
'foundLoader.getJNLPFile().getResources().getJARs(part)' is bit of a mouthful. Give it a temporary variable (or two :) perhaps ?
> + if (version != null && version.equals(eachJar.getVersion())) {
> + foundJars.add(eachJar);
> + }
> + else
> + foundJars.add(eachJar);
I must be missing something here. If (x) add jars else add jars ? Seems like the condition has no effect here.
> +
> + return (JARDesc[]) foundJars.toArray(new JARDesc[foundJars.size()]);
Cast here isn't actually necessary. It always returns the type of array passed to 'toArray'.
> + }
> +
> + }
> +
> + return null;
The code is a lot easier to follow if this returns new JarDesc[]{}, ie
an empty array, instead of null. This way you can consistently loop
over the returned result.
> + }
> +
> + /**
> + * Removes jars from cache.
> + *
> + * @param ref Path of the launch or extension JNLP File containing the
> + * resource. If null, main JNLP's file location will be used instead.
> + * @param jars Jars marked for removal.
> + */
> + public void removeCachedJars(URL ref, JARDesc[] jars) {
> + JNLPClassLoader foundLoader = this.getLoaderByJnlpUrl(ref);
> +
> + if (foundLoader != null)
> + foundLoader.removeJars(jars);
> + }
> +
> + /**
> + * Helps removeCachedJars() remove jars from the file system.
> + *
> + * @param jars Jars marked for removal.
> + */
> + protected void removeJars(JARDesc[] jars) {
> +
> + if (jars == null || jars.length <= 0)
I would be in favour of dropping this null check and instead making sure it gets an empty array consistently.
As well the <= 0 check isn't needed as the loop will simply occur 0 times if so. (I like having less code :)
> + return;
> +
> + for (int i = 0; i < jars.length; i++) {
Consider a for-each loop here, they're tastier.
> +
> + try{
> + tracker.removeResource(jars[i].getLocation());
> + } catch (Exception e) {
> + if (JNLPRuntime.isDebug()) {
> + System.err.println(e.getMessage());
> + System.err.println("Failed to remove resource from tracker, continuing..");
> + }
> + }
> +
> + File cachedFile = CacheUtil.getCacheFile(jars[i].getLocation(), null);
> + String directoryUrl = CacheUtil.getCacheParentDirectory(cachedFile.getAbsolutePath());
> +
> + File directory = new File(directoryUrl);
> +
> + if (JNLPRuntime.isDebug())
> + System.out.println("Deleting cached file: " + cachedFile.getAbsolutePath());
> +
> + cachedFile.delete();
> +
> + if (JNLPRuntime.isDebug())
> + System.out.println("Deleting cached directory: " + directory.getAbsolutePath());
> +
> + directory.delete();
> + }
> + }
> +
> + /**
> + * Downloads jars identified by part name.
> + *
> + * @param ref Path of the launch or extension JNLP File containing the
> + * resource. If null, main JNLP's file location will be used instead.
> + * @param part The name of the path.
> + */
> + public void downloadJars(URL ref, String part, Version version) {
> + JNLPClassLoader foundLoader = this.getLoaderByJnlpUrl(ref);
> +
> + if (foundLoader != null)
> + foundLoader.initializeNewJarDownload(ref, part, version);
> + }
> +
> + /**
> + * Downloads and initializes jars into this loader.
> + *
> + * @param ref Path of the launch or extension JNLP File containing the
> + * resource. If null, main JNLP's file location will be used instead.
> + * @param part The name of the path.
> + * @throws LaunchException
> + */
> + protected void initializeNewJarDownload(URL ref, String part, Version version) {
> + JARDesc[] jars = getJars(ref, part, version);
> +
> + if (jars != null && jars.length > 0)
With my null-eliminating suggestion, this check can be dropped entirely.
> + for (JARDesc eachJar : jars) {
> +
> + if (JNLPRuntime.isDebug())
> + System.out.println("Downloading and initializing jar: " + eachJar.getLocation().toString());
> +
> + this.addNewJar(eachJar, UpdatePolicy.FORCE);
> + }
> + }
> +
> + /**
> + * Manages DownloadService jars which are not mentioned in the JNLP file
> + * @param ref Path to the resource.
> + * @param version The version of resource. If null, no version is specified.
> + * @param action The action to perform with the resource. Either DOWNLOADTOCACHE, REMOVEFROMCACHE, or CHECKCACHE.
> + * @return true if CHECKCACHE and the resource is cached.
> + */
> + private boolean manageExternalJars(URL ref, String version, int action) {
> + boolean approved = false;
> + JNLPClassLoader foundLoader = this.getLoaderByResourceUrl(ref, version);
> + Version resourceVersion = (version == null) ? null : new Version(version);
> +
> + if (foundLoader != null) {
> + approved = true;
> + }
> + else if (ref.toString().startsWith(file.getCodeBase().toString()))
Nit: Consider consistent {} around each case here, or around none (I prefer the latter)
> + approved = true;
> + else if (SecurityDesc.ALL_PERMISSIONS.equals(security.getSecurityType()))
> + approved = true;
> +
> + if (approved) {
> + if (foundLoader == null)
> + foundLoader = this;
> +
> + if (action == DownloadAction.DOWNLOADTOCACHE) {
> + JARDesc jd = new JARDesc(ref, resourceVersion, null, false, true, false, true);
A temporary variable with an explanatory name could make it easier to tell what this JarDesc signifies.
> + if (JNLPRuntime.isDebug())
> + System.out.println("Downloading and initializing jar: " + ref.toString());
> +
> + foundLoader.addNewJar(jd);
> +
> + } else if (action == DownloadAction.REMOVEFROMCACHE) {
> + JARDesc[] jd = { new JARDesc(ref, resourceVersion, null, false, true, false, true) };
Similarly here.
> + foundLoader.removeJars(jd);
> + } else if (action == DownloadAction.CHECKCACHE) {
> + return CacheUtil.isCached(ref, resourceVersion);
> + }
> + }
> + return false;
> + }
> +
> + /**
> + * Downloads and initalizes resources which are not mentioned in the jnlp file.
> + * Used by DownloadService.
> + * @param ref Path to the resource.
> + * @param version The version of resource. If null, no version is specified.
> + */
> +
> + public void loadExternalResouceToCache(URL ref, String version) {
> + this.manageExternalJars(ref, version, DownloadAction.DOWNLOADTOCACHE);
> + }
> +
> + /**
> + * Removes resource which are not mentioned in the jnlp file.
> + * Used by DownloadService.
> + * @param ref Path to the resource.
> + * @param version The version of resource. If null, no version is specified.
> + */
> + public void removeExternalCachedResource(URL ref, String version) {
> + this.manageExternalJars(ref, version, DownloadAction.REMOVEFROMCACHE);
> + }
> +
> + /**
> + * Returns true if the resource (not mentioned in the jnlp file) is cached, otherwise false
> + * Used by DownloadService.
> + * @param ref Path to the resource.
> + * @param version The version of resource. If null, no version is specified.
> + * @return
> + */
> + public boolean isExternalResourceCached(URL ref, String version) {
> + return this.manageExternalJars(ref, version, DownloadAction.CHECKCACHE);
> + }
> +
> + /**
> * Decrements loader use count by 1
> *
> * If count reaches 0, loader is removed from list of available loaders
> @@ -2305,4 +2568,6 @@
> return null;
> }
> }
> +
> +
> }
> diff --git a/netx/net/sourceforge/jnlp/services/XDownloadService.java b/netx/net/sourceforge/jnlp/services/XDownloadService.java
> --- a/netx/net/sourceforge/jnlp/services/XDownloadService.java
> +++ b/netx/net/sourceforge/jnlp/services/XDownloadService.java
> @@ -20,6 +20,12 @@
> import java.net.*;
> import javax.jnlp.*;
>
> +import net.sourceforge.jnlp.JARDesc;
> +import net.sourceforge.jnlp.Version;
> +import net.sourceforge.jnlp.cache.CacheUtil;
> +import net.sourceforge.jnlp.runtime.JNLPClassLoader;
> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +
> /**
> * The DownloadService JNLP service.
> *
> @@ -28,10 +34,24 @@
> */
> class XDownloadService implements DownloadService {
>
> - protected XDownloadService() {
> + static class XDownloadServiceHelper {
> + JNLPClassLoader getClassLoader() {
> + return (JNLPClassLoader) JNLPRuntime.getApplication().getClassLoader();
> + }
I would prefer to simply have a package-private method as part of
XDownloadService called getClassLoader, for testing purposes. In my
opinion, the code would be simpler as this 'helper' class is difficult
to understand the purpose of.
> }
>
> - // comments copied from DownloadService interface
> + private XDownloadServiceHelper dsHelper;
> +
> + protected XDownloadService() {
I'd prefer 'package-private' over protected here. I don't think protected should be used in class that won't be inherited from.
> + this(new XDownloadServiceHelper());
> + }
> +
> + protected XDownloadService(XDownloadServiceHelper helper) {
> + dsHelper = helper;
> +
> + if (JNLPRuntime.isDebug())
> + System.out.println("Initalized Download Service");
> + }
>
> /**
> * Returns a listener that will automatically display download
> @@ -46,7 +66,19 @@
> * url and version) is cached locally.
> */
> public boolean isExtensionPartCached(URL ref, String version, String part) {
> - return true;
> + boolean allCached = true;
> + Version resourceVersion = (version == null) ? null : new Version(version);
> +
> + JARDesc[] jars = dsHelper.getClassLoader().getJars(ref, part, resourceVersion);
> +
> + if (jars == null || jars.length <= 0)
> + return false;
> +
> + for (int i = 0; i < jars.length && allCached; i++) {
> + allCached = CacheUtil.isCached(jars[i].getLocation(), resourceVersion);
> + }
> +
> + return allCached;
> }
>
> /**
> @@ -54,7 +86,14 @@
> * url and version) are cached locally.
> */
> public boolean isExtensionPartCached(URL ref, String version, String[] parts) {
> - return true;
> + boolean allCached = true;
> + if (parts.length <= 0)
> + return false;
> +
> + for (String eachPart : parts)
> + allCached = this.isExtensionPartCached(ref, version, eachPart);
> +
> + return allCached;
> }
>
> /**
> @@ -64,7 +103,17 @@
> * the application.
> */
> public boolean isPartCached(String part) {
> - return true;
> + boolean allCached = true;
> + JARDesc[] jars = dsHelper.getClassLoader().getJars(null, part, null);
> +
> + if (jars == null || jars.length <= 0)
> + return false;
> +
> + for (int i = 0; i < jars.length && allCached; i++) {
> + allCached = CacheUtil.isCached(jars[i].getLocation(), null);
> + }
> +
> + return allCached;
> }
>
> /**
> @@ -74,7 +123,14 @@
> * application.
> */
> public boolean isPartCached(String[] parts) {
> - return true;
> + boolean allCached = true;
> + if (parts.length <= 0)
> + return false;
> +
> + for (String eachPart : parts)
> + allCached = this.isPartCached(eachPart);
> +
> + return allCached;
> }
>
> /**
> @@ -83,7 +139,7 @@
> * application or extension.
> */
> public boolean isResourceCached(URL ref, String version) {
> - return true;
> + return dsHelper.getClassLoader().isExternalResourceCached(ref, version);
> }
>
> /**
> @@ -92,6 +148,8 @@
> * @throws IOException
> */
> public void loadExtensionPart(URL ref, String version, String[] parts, DownloadServiceListener progress) throws IOException {
> + for (String eachPart : parts)
> + this.loadExtensionPart(ref, version, eachPart, progress);
> }
>
> /**
> @@ -100,6 +158,8 @@
> * @throws IOException
> */
> public void loadExtensionPart(URL ref, String version, String part, DownloadServiceListener progress) throws IOException {
> + Version resourceVersion = (version == null) ? null : new Version(version);
> + dsHelper.getClassLoader().downloadJars(ref, part, resourceVersion);
> }
>
> /**
> @@ -108,6 +168,8 @@
> * @throws IOException
> */
> public void loadPart(String[] parts, DownloadServiceListener progress) throws IOException {
> + for (String eachPart : parts)
> + this.loadPart(eachPart, progress);
> }
>
> /**
> @@ -116,6 +178,7 @@
> * @throws IOException
> */
> public void loadPart(String part, DownloadServiceListener progress) throws IOException {
> + dsHelper.getClassLoader().downloadJars(null, part, null);
> }
>
> /**
> @@ -124,6 +187,7 @@
> * @throws IOException
> */
> public void loadResource(URL ref, String version, DownloadServiceListener progress) throws IOException {
> + dsHelper.getClassLoader().loadExternalResouceToCache(ref, version);
> }
>
> /**
> @@ -133,6 +197,9 @@
> * @throws IOException
> */
> public void removeExtensionPart(URL ref, String version, String part) throws IOException {
> + Version resourceVersion = (version == null) ? null : new Version(version);
> + JARDesc[] jars = dsHelper.getClassLoader().getJars(ref, part, resourceVersion);
> + dsHelper.getClassLoader().removeCachedJars(ref, jars);
> }
>
> /**
> @@ -142,6 +209,8 @@
> * @throws IOException
> */
> public void removeExtensionPart(URL ref, String version, String[] parts) throws IOException {
> + for (String eachPart : parts)
> + this.removeExtensionPart(ref, version, eachPart);
> }
>
> /**
> @@ -151,6 +220,8 @@
> * @throws IOException
> */
> public void removePart(String part) throws IOException {
> + JARDesc[] jars = dsHelper.getClassLoader().getJars(null, part, null);
> + dsHelper.getClassLoader().removeCachedJars(null, jars);
> }
>
> /**
> @@ -160,6 +231,8 @@
> * @throws IOException
> */
> public void removePart(String[] parts) throws IOException {
> + for (String eachPart : parts)
> + this.removePart(eachPart);
> }
>
> /**
> @@ -169,6 +242,7 @@
> * @throws IOException
> */
> public void removeResource(URL ref, String version) throws IOException {
> + dsHelper.getClassLoader().removeExternalCachedResource(ref, version);
> }
>
> }
Good patch overall, thanks for filling out this missing piece of
functionality!
Cheers,
- Adam
More information about the distro-pkg-dev
mailing list