[icedtea-web] RFC: PR618 - Can't install OpenDJ, JavaWebStart fails with Input stream is null error.

[Omair Majid]

On 03/17/2011 11:00 AM, Omair Majid wrote:
> 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.
>
Ping. Any thoughts or comments?

Cheers,
Omair