[icedtea-web] RFC: Patch to fix PR895

[Jiri Vanek]

On 03/10/2012 02:08 AM, Omair Majid wrote:
> 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.
It looks to me like it is trying to fix this issue a bit.
>
>> 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 would liek to help with this reproducer, But I'm not sue if this are right  perquisites for this to happen:
Application is started correctly
Application tries to use some class, which was not used before. This Class is not found (even after all lazy jars are collected)
ClassNotFound is thrown
Application tries to use this class again, again all jars are searched for and ClassNotFound is thrown.
...
With fix
Application is started correctly
Application tries to use some class, which was not used before. This Class is not found (even after all lazy jars are collected), and full pa.cka.ge.name is stored
ClassNotFound is thrown
Application tries to use this class again, if class found in recorded ones, then CNF is throw again and nothing is searched for, else deep search again

If I'm correct - is this really worthy to fix? It is saving some time for application which do not deserve to live! - unless the dependences jars are downlaoded again - but then something else must be fixed.
If it still need to have reproducer in this way, then it is not hard to do the test with outputs as they are intorduced in this fix.


>
> 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
- This is nasty trick to modify classpath in runtime!
> - 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;
>    }
> }
>

One nitpick - You are using copyonwritearraylist - most synchronised list.  But I loosk to me like this is much more issue for some synchronised set which will do the same with much less overhed - or not?
And...  CopyOnWriteArrayList<String> notFoundClasses = new CopyOnWriteArrayList<String>() fuuuuu :) ... Why not List<String> notFoundClasses = new CopyOnWriteArrayList<String>() ?

Best regards
J.
> Cheers,
> Omair