[icedtea-web] RFC: PR618 - Can't install OpenDJ, JavaWebStart fails with Input stream is null error.
Omair Majid
omajid at redhat.com
Thu Mar 17 08:00:17 PDT 2011
On 03/16/2011 04:40 PM, Dr Andrew John Hughes wrote:
> On 11:07 Mon 14 Mar , Omair Majid wrote:
>> > On 01/26/2011 04:44 PM, Omair Majid wrote:
>>> > > On 01/25/2011 07:03 PM, Dr Andrew John Hughes wrote:
>>>> > >> On 14:42 Tue 25 Jan , Omair Majid wrote:
>>>>> > >>> Hi,
>>>>> > >>>
>>>>> > >>> The attached patch fixes PR618. The issue is that jars marked as lazy
>>>>> > >>> are not currently searched for resources other than classes. So loading
>>>>> > >>> classes from lazy jars works, but loading anything else (zip files in
>>>>> > >>> the case of the bug report) fails.
>>>>> > >>>
>>>>> > >>> The attached patch fixes findResources to use lazy jars if needed.
>>>>> > >>>
>>>>> > >>> URLClassLoader invokes findResource() as a part of its getResource()
>>>>> > >>> implementation. Instead of duplicating the addition of lazy jars in
>>>>> > >>> getResource(), getResource() was removed, and findResource added
>>>>> > >>> instead. findResource() now delegates to findResources(), so there is
>>>>> > >>> only one place where an actual search for resources is performed.
>>>>> > >>>
>>>>> > >>> Any concerns or comments?
>>>>> > >>>
>>>> > >>
>>>>> > >>> diff -r 64da2a80df88
>>>>> > >>> netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>>>>> > >>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Tue Jan
>>>>> > >>> 25 10:19:20 2011 -0500
>>>>> > >>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Tue Jan
>>>>> > >>> 25 14:42:07 2011 -0500
>>>>> > >>> @@ -1086,27 +1087,42 @@
>>>>> > >>> * Finds the resource in this, the parent, or the extension
>>>>> > >>> * class loaders.
>>>>> > >>> */
>>>>> > >>> - public URL getResource(String name) {
>>>>> > >>> - URL result = super.getResource(name);
>>>>> > >>> -
>>>>> > >>> - for (int i = 1; i< loaders.length; i++)
>>>>> > >>> - if (result == null)
>>>>> > >>> - result = loaders[i].getResource(name);
>>>>> > >>> -
>>>>> > >>> - return result;
>>>>> > >>> + @Override
>>>>> > >>> + public URL findResource(String name) {
>>>>> > >>> + try {
>>>>> > >>> + return findResources(name).nextElement();
>>>>> > >>> + } catch (NoSuchElementException e) {
>>>>> > >>> + return null;
>>>>> > >>> + } catch (IOException e) {
>>>>> > >>> + return null;
>>>>> > >>> + }
>>>> > >>
>>>> > >> Would it be more efficient to check if there are more elements first
>>>> > >> rather than throwing and catching an exception?
>>>> > >>
>>> > >
>>> > > I am not sure about efficiency, but the attached patch fixes it.
>>> > >
> Ok. I think it would be more efficient than generating an exception.
>
>>>>> > >>> @@ -1116,7 +1132,7 @@
>>>>> > >>> resources.add(e.nextElement());
>>>>> > >>> }
>>>>> > >>>
>>>>> > >>> - return resources.elements();
>>>>> > >>> + return resources;
>>>>> > >>> }
>>>> > >>
>>>> > >> Is findResources the only consumer of the resources collection? If so,
>>>> > >> it would be
>>>> > >> more efficient to use an ArrayList or LinkedList and avoid the
>>>> > >> implicit synchronisation
>>>> > >> of Vector.
>>>> > >>
>>> > >
>>> > > Yup, findResources is the only method using it. I did not want to touch
>>> > > original code (at least code that worked), so I left the vector as it
>>> > > was. I have changed it now.
>>> > >
>>>> > >> What is the reason for returning a List from findResourcesBySearching
>>>> > >> rather than just
>>>> > >> an Iterator/Enumeration? AFAICS, you just create an enumeration from
>>>> > >> it anyway and
>>>> > >> hasNext()/hasNextElement() would tell you if there are no entries.
>>>> > >>
>>> > >
>>> > > Is using an Iterator/Enumeration better than using a List? I am not
>>> > > aware of the differences, and findResourcesBySearching() already creates
>>> > > a List that I can return. So I thought I would avoid creating an
>>> > > Enumeration based on it until necessary. The attached patch changes this
>>> > > bit too, though I am not sure I see the advantage.
>>> > >
>> >
>> > Patch updated to apply to head.
>> >
> It restricts what the receiver can do with the List. With just an Iterator,
> they can only look at and remove elements (and you can disable the latter too
> if necessary). With a List, they have a lot more access they probably don't need.
>
I see your point, but findResourcesBySearching is private, so it
shouldnt be a problem if it exposes more information.
>> > Thanks,
>> > Omair
>> > diff -r 5454292b3fae netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> > --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu Mar 10 15:42:01 2011 -0500
>> > +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Mon Mar 14 11:07:15 2011 -0400
>> > @@ -40,7 +40,6 @@
>> > import java.util.List;
>> > import java.util.Map;
>> > import java.util.TreeSet;
>> > -import java.util.Vector;
>> > import java.util.jar.JarEntry;
>> > import java.util.jar.JarFile;
>> >
>> > @@ -1123,31 +1122,54 @@
>> > /**
>> > * Finds the resource in this, the parent, or the extension
>> > * class loaders.
>> > + *
>> > + * @return a<code>URL</code> for the resource, or<code>null</code>
>> > + * if the resource could not be found.
>> > */
>> > - public URL getResource(String name) {
>> > - URL result = super.getResource(name);
>> > + @Override
>> > + public URL findResource(String name) {
>> > + URL result = null;
>> >
>> > - for (int i = 1; i< loaders.length; i++)
>> > - if (result == null)
>> > - result = loaders[i].getResource(name);
>> > + try {
>> > + Enumeration<URL> e = findResources(name);
>> > + if (e.hasMoreElements()) {
>> > + result = e.nextElement();
>> > + }
>> > + } catch (IOException e) {
>> > + // continue
>> > + }
>> >
>> > // If result is still null, look in the codebase loader
>> > if (result == null&& codeBaseLoader != null)
>> > - result = codeBaseLoader.getResource(name);
>> > + result = codeBaseLoader.findResource(name);
>> >
>> > return result;
>> > }
>> >
>> > /**
>> > - * Finds the resource in this, the parent, or the extension
>> > + * Find the resources in this, the parent, or the extension
>> > * class loaders.
>> > */
>> > @Override
>> > public Enumeration<URL> findResources(String name) throws IOException {
>> > - Vector<URL> resources = new Vector<URL>();
>> > + Enumeration<URL> resources = findResourcesBySearching(name);
>> > +
>> > + // if not found, load all lazy resources; repeat search
>> > + while (!resources.hasMoreElements()&& addNextResource() != null) {
>> > + resources = findResourcesBySearching(name);
>> > + }
>> > +
>> > + return resources;
>> > + }
>> > +
>> > + private Enumeration<URL> findResourcesBySearching(String name) throws IOException {
>> > + List<URL> resources = new ArrayList<URL>();
>> > Enumeration<URL> e;
>> >
>> > for (int i = 0; i< loaders.length; i++) {
>> > + // TODO check if this will blow up or not
>> > + // if loaders[1].getResource() is called, wont it call getResource() on
>> > + // the original caller? infinite recursion?
>> >
>> > if (loaders[i] == this)
>> > e = super.findResources(name);
>> > @@ -1166,7 +1188,7 @@
>> > resources.add(e.nextElement());
>> > }
>> >
>> > - return resources.elements();
>> > + return Collections.enumeration(resources);
> At the top, resources has a type List<URL>. So why not just call resources.iterator()?
>
The return type of findResources() has to be an Enumeration (since its
defined by the parent class). If I return an Iterator from
findResourcesBySearching, I will have to perform additional computation
in findResources() to convert the Iterator into an Enumeration. To keep
things simple, I return an enumeration.
Thanks,
Omair
More information about the distro-pkg-dev
mailing list