[javaflow] class loader

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[javaflow] class loader

Kohsuke Kawaguchi

I noticed that in ContinuationClassLoader you aren't actually using the
marker interface to decide what to instrument any more. Is this by
design? I also noticed a similar comment in the TODO file.

If so, I can remove the Continuable interface.

I still see some value in retaining the ContinuationCapable interface,
as this seems to be something we can use for detecting errors at
runtime. But since ContinuationCapable doesn't need to be visible to
users, I wonder if we can move it to the bytecode subpackage.


Another thing about ContinuationClassLoader. Right now this class is
hard-coded to recognize the javaflow test packages, but this needs to be
parameterized. Also, in JUnit, the list of "excluded packages" can be
read from a data file [1]. I suggest we implement something like that. I
don't think it makes sense for this file to be a property file, so I
think we can just read this as an UTF-8 encoded text file where each
line specifies a new mask. Let me know how you think about me making
those changes.

BTW, I found this JUnit implementation interesting. It doesn't seem to
exclude any of the java.* or org.w3c.*, but it must be clearly working.
I never used the ClassLoader.findLoadedClass method before, but does
this method load all the classes in the bootstrap classloader?

Personally, I find the current ContinuationClassLoader bit too
error-prone. Because the parent ClassLoader can always load the same
class that a child ContinuationClassLoader can load, you can easily get
two Class objects that have the same name if you are not careful. This
happens, for example, if class X (which is outside the
'includeInRewriting' list) refers to class Y (which is inside the
'includeInRewriting' list.) When you later tries to load Y through the
ContinuationClassLoader, you get another copy Y'.

In my day job, I saw several JAXB users who face this kind of issues
while using JAXB in JUnit, and these issues are very hard to diagnose.


For this reason, I wonder if we can have another ClassLoader that works
more like a traditional URLClassLoader --- you tell it a set of
locations to look at, then the class loader loads a class file from it
with byte-code instrumentation. This requires a developer to have
separate location for a host and the code that runs inside the
continuation environment, but it eliminates a danger of having two
copies of the same class.

This class loader is useful for things like agent container or workflow
engines. URLClassLoader isn't reusable for us, but I think we can start
from AntClassLoader.



[1]
http://cvs.sourceforge.net/viewcvs.py/junit/junit/junit/runner/TestCaseClassLoader.java?rev=1.6&view=auto
--
Kohsuke Kawaguchi

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Torsten Curdt

> I noticed that in ContinuationClassLoader you aren't actually using  
> the marker interface to decide what to instrument any more. Is this  
> by design? I also noticed a similar comment in the TODO file.

Yes, I am no big fan of empty marker interfaces.
I'd prefer a configuration approach.

> If so, I can remove the Continuable interface.

Sure

> I still see some value in retaining the ContinuationCapable  
> interface, as this seems to be something we can use for detecting  
> errors at runtime. But since ContinuationCapable doesn't need to be  
> visible to users, I wonder if we can move it to the bytecode  
> subpackage.

Makes sense, too ...maybe *that* interface
should be called Continuable then. WDYT?

> Another thing about ContinuationClassLoader. Right now this class  
> is hard-coded to recognize the javaflow test packages, but this  
> needs to be parameterized.

Yes, that needs to change

> Also, in JUnit, the list of "excluded packages" can be read from a  
> data file [1]. I suggest we implement something like that. I don't  
> think it makes sense for this file to be a property file, so I  
> think we can just read this as an UTF-8 encoded text file where  
> each line specifies a new mask. Let me know how you think about me  
> making those changes.

Not sure how...that's why I haven't done
it yet. I'd rather like to use a more
sophisticated configuration approach.

Usually I love the configuration classes
from Avalon ...but I don't want to have
that dependency.

...question is also how it can be handled
when the classloader is being used as
system classloader.

> BTW, I found this JUnit implementation interesting. It doesn't seem  
> to exclude any of the java.* or org.w3c.*, but it must be clearly  
> working.

But it does...

     private String[] excludeFromLoading = {
         "java.",
         "javax.",
         "sun.",
         "org.xml.",
         "org.w3c.",
         "org.apache.commons.logging"
     };

...only the compiling one does not because
it's using parent-last delegation and the
repository of classes is restricted to the
user's classes.

> Personally, I find the current ContinuationClassLoader bit too  
> error-prone. Because the parent ClassLoader can always load the  
> same class that a child ContinuationClassLoader can load, you can  
> easily get two Class objects that have the same name if you are not  
> careful.

Well ...we cannot necessarily prevent classes
from being loaded through the parent classloader.

> In my day job, I saw several JAXB users who face this kind of  
> issues while using JAXB in JUnit, and these issues are very hard to  
> diagnose.

Sure ...classloader stuff is always tricky.

> For this reason, I wonder if we can have another ClassLoader that  
> works more like a traditional URLClassLoader --- you tell it a set  
> of locations to look at, then the class loader loads a class file  
> from it with byte-code instrumentation. This requires a developer  
> to have separate location for a host and the code that runs inside  
> the continuation environment, but it eliminates a danger of having  
> two copies of the same class.

We should be able to achieve the same by
restricting the classloader to those
included packages. Don't you think?

I am not yet convinced that a separate
location is that convenient.

> URLClassLoader isn't reusable for us, but I think we can start from  
> AntClassLoader.

Well ...if you really think that's
required. As long as it works... :)

cheers
--
Torsten

PGP.sig (193 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Kohsuke Kawaguchi
Torsten Curdt wrote:

>> I noticed that in ContinuationClassLoader you aren't actually using  
>> the marker interface to decide what to instrument any more. Is this  
>> by design? I also noticed a similar comment in the TODO file.
>
> Yes, I am no big fan of empty marker interfaces.
> I'd prefer a configuration approach.
>
>> If so, I can remove the Continuable interface.
>
> Sure

OK.

>> I still see some value in retaining the ContinuationCapable  
>> interface, as this seems to be something we can use for detecting  
>> errors at runtime. But since ContinuationCapable doesn't need to be  
>> visible to users, I wonder if we can move it to the bytecode  
>> subpackage.
>
> Makes sense, too ...maybe *that* interface
> should be called Continuable then. WDYT?

Will make that change, too.


>> Another thing about ContinuationClassLoader. Right now this class  
>> is hard-coded to recognize the javaflow test packages, but this  
>> needs to be parameterized.
>
> Yes, that needs to change
>
>> Also, in JUnit, the list of "excluded packages" can be read from a  
>> data file [1]. I suggest we implement something like that. I don't  
>> think it makes sense for this file to be a property file, so I  
>> think we can just read this as an UTF-8 encoded text file where  
>> each line specifies a new mask. Let me know how you think about me  
>> making those changes.
>
> Not sure how...that's why I haven't done
> it yet. I'd rather like to use a more
> sophisticated configuration approach.
>
> Usually I love the configuration classes
> from Avalon ...but I don't want to have
> that dependency.
>
> ..question is also how it can be handled
> when the classloader is being used as
> system classloader.

I don't think we can even install ContinuationClassLoader as the system
class loader. I thought the system class loader is set up by JVM before
any Java code runs.



>> BTW, I found this JUnit implementation interesting. It doesn't seem  
>> to exclude any of the java.* or org.w3c.*, but it must be clearly  
>> working.
>
> But it does...
>
>      private String[] excludeFromLoading = {
>          "java.",
>          "javax.",
>          "sun.",
>          "org.xml.",
>          "org.w3c.",
>          "org.apache.commons.logging"
>      };
>
> ..only the compiling one does not because
> it's using parent-last delegation and the
> repository of classes is restricted to the
> user's classes.

No, I meant that the JUnit TestCaseClassLoader doesn't have an exclusion
list like you have in ContinuationClassLoader. That must mean that
somehow the explicit exclusion code isn't necessary for things to work
correctly. I was wondering why.




>> For this reason, I wonder if we can have another ClassLoader that  
>> works more like a traditional URLClassLoader --- you tell it a set  
>> of locations to look at, then the class loader loads a class file  
>> from it with byte-code instrumentation. This requires a developer  
>> to have separate location for a host and the code that runs inside  
>> the continuation environment, but it eliminates a danger of having  
>> two copies of the same class.
>
> We should be able to achieve the same by
> restricting the classloader to those
> included packages. Don't you think?
>
> I am not yet convinced that a separate
> location is that convenient.

In the scenario I explained in the previous e-mail, the benefit that the
separate location brings is that you get NoClassDefError for Y instead
of strange ClassCastException trying to cast Y' into Y.

NoClassDefError tends to happen a lot earlier than ClassCastException,
and it's usually easier for people to grok.


>> URLClassLoader isn't reusable for us, but I think we can start from  
>> AntClassLoader.
>
> Well ...if you really think that's
> required. As long as it works... :)

I think I'll implement it, because I need it anyway. If eventually you
decide that you don't like it, we can talk.

--
Kohsuke Kawaguchi

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Torsten Curdt

> I don't think we can even install ContinuationClassLoader as the  
> system class loader. I thought the system class loader is set up by  
> JVM before any Java code runs.

I have been using other code rewriting classloaders
as system classloaders. No problem. I think what
you are referring to is the bootstrap classloader.


> No, I meant that the JUnit TestCaseClassLoader doesn't have an  
> exclusion list like you have in ContinuationClassLoader. That must  
> mean that somehow the explicit exclusion code isn't necessary for  
> things to work correctly. I was wondering why.

Ah ...no idea about this TestCaseClassLoader.
Never used it before. I think it depends on
the delegation model whether you need those
exclusions or not. If it's parent-first you
don't need it.

>>> For this reason, I wonder if we can have another ClassLoader  
>>> that  works more like a traditional URLClassLoader --- you tell  
>>> it a set  of locations to look at, then the class loader loads a  
>>> class file  from it with byte-code instrumentation. This requires  
>>> a developer  to have separate location for a host and the code  
>>> that runs inside  the continuation environment, but it eliminates  
>>> a danger of having  two copies of the same class.
>>>
>> We should be able to achieve the same by
>> restricting the classloader to those
>> included packages. Don't you think?
>> I am not yet convinced that a separate
>> location is that convenient.
>>
>
> In the scenario I explained in the previous e-mail, the benefit  
> that the separate location brings is that you get NoClassDefError  
> for Y instead of strange ClassCastException trying to cast Y' into Y.
>
> NoClassDefError tends to happen a lot earlier than  
> ClassCastException, and it's usually easier for people to grok.
Ah ...now I see ...because the parent
cannot load the class anymore. ok.

> I think I'll implement it, because I need it anyway. If eventually  
> you decide that you don't like it, we can talk.

No worries ...just go ahead

cheers
--
Torsten


PGP.sig (193 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Kohsuke Kawaguchi
Torsten Curdt wrote:
>> I don't think we can even install ContinuationClassLoader as the  
>> system class loader. I thought the system class loader is set up by  
>> JVM before any Java code runs.
>
> I have been using other code rewriting classloaders
> as system classloaders. No problem. I think what
> you are referring to is the bootstrap classloader.

I'd love to know how you do this.

>> I think I'll implement it, because I need it anyway. If eventually  
>> you decide that you don't like it, we can talk.
>
> No worries ...just go ahead

I implemented this and put this back as "ContinuationURLClassLoader".
I'm sure there's a better name, though.

--
Kohsuke Kawaguchi

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Torsten Curdt
>> I have been using other code rewriting classloaders
>> as system classloaders. No problem. I think what
>> you are referring to is the bootstrap classloader.
>>
>
> I'd love to know how you do this.

java -Djava.system.class.loader=<name of the class>

see http://java.sun.com/j2se/1.4.2/docs/api/java/lang/ClassLoader.html

>> No worries ...just go ahead
>>
>
> I implemented this and put this back as  
> "ContinuationURLClassLoader". I'm sure there's a better name, though.

Actually I think we should *replace* the old
one. We should make sure the API does not
provide too many options.

I'd say:
if it does what it should - replace it.

Couldn't we then maybe also extend from
URLClassLoader?

cheers
--
Torsten


PGP.sig (193 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Kohsuke Kawaguchi-2
Torsten Curdt wrote:
> java -Djava.system.class.loader=<name of the class>
>
> see http://java.sun.com/j2se/1.4.2/docs/api/java/lang/ClassLoader.html

Aaa, there's always something new to learn.

>>> No worries ...just go ahead
>>>
>>
>> I implemented this and put this back as  
>> "ContinuationURLClassLoader". I'm sure there's a better name, though.
>
> Actually I think we should *replace* the old
> one. We should make sure the API does not
> provide too many options.

I generally agree with this.

> I'd say:
> if it does what it should - replace it.

OK, I will do this. But you are aware that it doesn't completely subsume
the functionality of the current ContinuationClassLoader, right?

> Couldn't we then maybe also extend from URLClassLoader?

It would have been great if I could have done this, but no, as far as I
can tell, URLClassLoader doesn't have a hook we can use to insert the
class file transformation.

--
Kohsuke Kawaguchi
Sun Microsystems                   [hidden email]

smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Kohsuke Kawaguchi
Kohsuke Kawaguchi wrote:
>> I'd say:
>> if it does what it should - replace it.
>
> OK, I will do this. But you are aware that it doesn't completely subsume
> the functionality of the current ContinuationClassLoader, right?

In particular, I don't think I can easily replace the unit test set up
with the new class loader...


--
Kohsuke Kawaguchi

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Torsten Curdt
In reply to this post by Kohsuke Kawaguchi-2

> Aaa, there's always something new to learn.

...there always is :)


>> Actually I think we should *replace* the old
>> one. We should make sure the API does not
>> provide too many options.
>>
>
> I generally agree with this.

Good :)

>> I'd say:
>> if it does what it should - replace it.
>>
>
> OK, I will do this. But you are aware that it doesn't completely  
> subsume the functionality of the current ContinuationClassLoader,  
> right?

I am aware of that ...frankly speaking
I think the ReloadingClassLoader in jci
in combination with the RewritingResourceStore
is usually a better approach. (see also
the ContinuationCompilingClassLoader)

The ContinuationClassLoader might make
sense in some cases but I would always
go for the asynchronous approach - if possible.
But for sure this depends on the usecase.

I think your changes makes sense.

>> Couldn't we then maybe also extend from URLClassLoader?
>>
>
> It would have been great if I could have done this, but no, as far  
> as I can tell, URLClassLoader doesn't have a hook we can use to  
> insert the class file transformation.

We could completely override the methods.
This would help easier jetty integration
e.g. Please see

http://issues.apache.org/bugzilla/show_bug.cgi?id=36073

cheers
--
Torsten


PGP.sig (193 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Kohsuke Kawaguchi
Torsten Curdt wrote:

>>> Couldn't we then maybe also extend from URLClassLoader?
>>>
>>
>> It would have been great if I could have done this, but no, as far  
>> as I can tell, URLClassLoader doesn't have a hook we can use to  
>> insert the class file transformation.
>
> We could completely override the methods.
> This would help easier jetty integration
> e.g. Please see
>
> http://issues.apache.org/bugzilla/show_bug.cgi?id=36073

OK. What I can do is to completely replace the findClass method by using
the URLCassLoader.findResource.

It won't be able to support package sealing nor can it easily define
java.lang.Package correctly, but that applies to the current
AntClassLoader-based one, so it's no worse.

--
Kohsuke Kawaguchi

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Torsten Curdt
>
> OK. What I can do is to completely replace the findClass method by  
> using the URLCassLoader.findResource.

aehem... please don't copy-and-paste.
AFAIK we cannot do this because of
sun's license restrictions.

> It won't be able to support package sealing nor can it easily  
> define java.lang.Package correctly, but that applies to the current  
> AntClassLoader-based one, so it's no worse.

yeah, I wouldn't worry too much
about that.

cheers
--
Torsten


PGP.sig (193 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Kohsuke Kawaguchi
Torsten Curdt wrote:
>>
>> OK. What I can do is to completely replace the findClass method by  
>> using the URLCassLoader.findResource.
>
> aehem... please don't copy-and-paste.
> AFAIK we cannot do this because of
> sun's license restrictions.

I believe I only cut-and-paste from AntClassLoader, not URLClassLoader,
but now that you mentioned it, I'm no longer too sure. Did you spot any
code that look like from URLClassLoader?

Should I just throw it away and reimplement it from scratch, just to be
safe?


--
Kohsuke Kawaguchi

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Torsten Curdt
>> aehem... please don't copy-and-paste.
>> AFAIK we cannot do this because of
>> sun's license restrictions.
>>
>
> I believe I only cut-and-paste from AntClassLoader, not  
> URLClassLoader, but now that you mentioned it, I'm no longer too  
> sure. Did you spot any code that look like from URLClassLoader?

I haven't checked yet.

> Should I just throw it away and reimplement it from scratch, just  
> to be safe?

Well, it would be better if you are not sure

cheers
--
Torsten

PGP.sig (193 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: [javaflow] class loader

James Carman
Out of curiosity, how would Sun know if you copy/pasted rather than writing
from scratch, assuming you use the same variable/method names or you
actually did copy/paste and changed some variable/method names?  I don't
know the legality of how that all works.  I'm just wondering.  How would
they actually prove a violation when you're writing code that essentially
does the same exact thing?

-----Original Message-----
From: Torsten Curdt [mailto:[hidden email]]
Sent: Monday, August 15, 2005 2:19 PM
To: Jakarta Commons Developers List
Subject: Re: [javaflow] class loader

>> aehem... please don't copy-and-paste.
>> AFAIK we cannot do this because of
>> sun's license restrictions.
>>
>
> I believe I only cut-and-paste from AntClassLoader, not  
> URLClassLoader, but now that you mentioned it, I'm no longer too  
> sure. Did you spot any code that look like from URLClassLoader?

I haven't checked yet.

> Should I just throw it away and reimplement it from scratch, just  
> to be safe?

Well, it would be better if you are not sure

cheers
--
Torsten



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Torsten Curdt


> Out of curiosity, how would Sun know if you copy/pasted rather than  
> writing
> from scratch, assuming you use the same variable/method names or you
> actually did copy/paste and changed some variable/method names?  I  
> don't
> know the legality of how that all works.  I'm just wondering.  How  
> would
> they actually prove a violation when you're writing code that  
> essentially
> does the same exact thing?
>
Doing the same and looking the same are two
different things ...IANAL but in order to
avoid any possible problems it's probably
easier to just implement it from scratch
...or make it based on the ant code.

Over at legal-discuss you should be able to
get more information if you are interested.

cheers
--
Torsten



PGP.sig (193 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [javaflow] class loader

Kohsuke Kawaguchi
In reply to this post by Torsten Curdt
Torsten Curdt wrote:

>>> aehem... please don't copy-and-paste.
>>> AFAIK we cannot do this because of
>>> sun's license restrictions.
>>>
>>
>> I believe I only cut-and-paste from AntClassLoader, not  
>> URLClassLoader, but now that you mentioned it, I'm no longer too  
>> sure. Did you spot any code that look like from URLClassLoader?
>
> I haven't checked yet.

I assumed that you spotted some similarities.

I checked the ContinuationClassLoader code again. I was able to track
the origin of the entire code. They all came from AntClassLoader, with
an exception of getResource and findClass, which are written by me. I'm
confident that none of them came from URLClassLoader.

So I believe there's no tainting.


--
Kohsuke Kawaguchi

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]