[icedtea-web] RFC: Patch to fix PR895

[Deepak Bhole]

* Omair Majid <omajid at redhat.com> [2012-03-09 20:08]:
> On 03/09/2012 05:19 PM, Deepak Bhole wrote:
> > Hi,
> > 
> > Attached patch fixes PR895:
> > "IcedTea-Web searches for missing classes on each loadClass or findClass"
> > 
> > http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=895
> 
> The bug doesn't seem to have any additional information. Is there a test
> case or a reproducer? Do you know why the server is being hammered? If
> the actual issue is under our control (for example, we are attempting to
> re-download stuff we already downloaded), it might be better to fix this
> underlying issue.
> 

Yes, this one:
http://jdk6.java.net/nonav/plugin2/liveconnect/LiveConnectTests/

Without the patch it takes 2+ minutes to run.. with patch, it is down to
about 15 seconds.

> > ChangeLog:
> > 2012-03-09  Deepak Bhole <dbhole at redhat.com>
> > 
> >     * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: Added new
> >     notFoundClasses list.
> >     (loadClass): Record if a class is not found and on next call for that
> >     class, return immediately.
> >     (findClass): Same.
> > 
> 
> Can you please include a test case for icedtea-web? I think it would be
> nice if we could tell if/when this issue reappears later. I would be
> fine with either unit tests or jnlp tests.
> 

I am not sure if I can. It is really only an issue with the
CodeBaseClassLoader, since looking into local jars multiple times has
comparatively trivial cost. CBCL is only used with the plug-in and not
webstart and I don't think the plugin test framework is fully in place
yet. Jiri?

> Some other thoughts:
> 
> Won't this patch break applications that use the following pattern?
> (Assume the application has enough permissions to download the jar)
> - Try and load a class
> - If that fails, download a new jar
> - Add new jar to classpath
> - Try to load the class again.
> 
> I am not sure how many applications do this; perhaps it's a non-issue.
> 
> The pattern of adding a new line before every return looks scary. I am
> quite sure the next person to touch this method and add a new return
> will miss adding the code to update notFoundClasses. How about something
> like this instead:
> 
> - rename findClass to findClassNoCacheCheck (or a better name ;) )
> - add a new method findClass():
> 
> protected Class findClass(String name) throws ClassNotFoundException {
>   if (notFoundClasses.contains(name)) {
>     throw new ClassNotFoundException;
>   }
> 
>   try {
>     Class klass = findClassNoCacheCheck(name);
>     return klass;
>   } catch (ClassNotFoundException cnfe) {
>     notFoundClasses.add(name);
>     throw cnfe;
>   }
> }
> 

While writing the above, I realized that it is really just CBCL that we
want to fix. What about moving the logic just into CBCL? See attached
patch.

In most cases, notFoundResources.get() will return null so I don't think
that the Array comparisons will add a significant delay.

Thanks,
Deepak