Re: useSystemClassLoader problems

[Brett Porter <br...@apache.org>]

Sorry, read these out of order with my other response :)

On 01/06/2007, at 2:29 AM, Kenney Westerhof wrote:

> I'll have to see what the issues are, but there are some usecases  
> for flat classloading
> apps, where surefire needs everything in the system classloader.  
> I'm sure you understand
> the need for this feature - i've tried to convince people otherwise  
> but finally saw that
> it was a valid usecase.

Ok, makes sense. It's more consistent with how IDEs run it too.

However, I'm rethinking setting it to default (which I did) - seems  
best to retain it as false. I did it because of another regression  
where clases in the system classloader weren't found in tests by  
default any more. I think that's just an omission in creating the  
isolated classloader that can be fixed differently.

>> On 25/05/2007, at 5:01 PM, Brett Porter wrote:
>>> I'm starting to find a number of bugs related to  
>>> useSystemClassLoader, by virtue of it being a forked JAR call.  
>>> I'd be interested to get more input on why this route was taken  
>>> and how the classloading in general works now (would be a handy  
>>> document to have).
>
> See above for the reason of this solution.
>
> Which bugs exactly?

There were a couple I fixed up already (not working on windows  
because of the urls in the manifest, seem to be fine now which is the  
urlutils thing I used - hope it still works for you). There are 334  
(easy to fix) and 335 (doesn't look critical).

I'd definitely be interested in getting together a doc on how the  
classloading works though. I can certainly do it by investigation,  
but if you have any thoughts you could jot down that'd be great :)

>
>>>> Also, can you check whether http://jira.codehaus.org/browse/ 
>>>> SUREFIRE-117? 
>>>> page=com.atlassian.jira.plugin.system.issuetabpanels:all- 
>>>> tabpanel should be closed as fixed as it was originally, and a  
>>>> new issue created for their problems, or if it wasn't in fact  
>>>> fixed?
>
> It was fixed; i asked for feedback there. No idea why it doesn't  
> work for them.
> I do however know there were issues with the surefire plugin, but  
> haven't seen those problems since.

Cool, I'll take another look.

Cheers,
Brett

Re: useSystemClassLoader problems

[Brett Porter <br...@apache.org>]

On 04/06/2007, at 10:20 PM, Kenney Westerhof wrote:

>
>
> Brett Porter wrote:
>> Thanks Kenney!
>> Please make sure to merge that up to the 2.3.x branch - this will  
>> close out SUREFIRE-335.
>
> Merged trunk to branch. I merged the last 3 revisions from trunk;  
> the one before
> that was a merge from you from branch to trunk. It cleanly installs  
> so I hope
> I merged the correct things.

Perfect, thanks. Sorry, I'd been doing it back to front for a bit :)

>
> Haven't deployed it yet; I'd rather let you do it after you check  
> SUREFIRe-335.

Seems to be exactly what you've fixed. I'll give it a spin.

Cheers,
Brett

Re: useSystemClassLoader problems

[Kenney Westerhof <ke...@apache.org>]

Brett Porter wrote:
> Thanks Kenney!
> 
> Please make sure to merge that up to the 2.3.x branch - this will close 
> out SUREFIRE-335.

Merged trunk to branch. I merged the last 3 revisions from trunk; the one before
that was a merge from you from branch to trunk. It cleanly installs so I hope
I merged the correct things.

Haven't deployed it yet; I'd rather let you do it after you check SUREFIRe-335.

-- Kenney

> 
> Cheers,
> Brett
> 
> On 04/06/2007, at 9:43 PM, Kenney Westerhof wrote:
> 
>>
>>
>> Kenney Westerhof wrote:
>>
>>> Brett Porter wrote:
>>
>> [snip]
>>
>>>> However, I'm rethinking setting it to default (which I did) - seems 
>>>> best to retain it as false. I did it because of another regression 
>>>> where clases in the system classloader weren't found in tests by 
>>>> default any more. I think that's just an omission in creating the 
>>>> isolated classloader that can be fixed differently.
>>
>> I'm setting it back to false now.
>>
>> I found the cause of the regression - due to the fact that the default 
>> for useSystemClassloader was true,
>> this bug was made evident.
>>
>> The combination of forkmode=never and useSystemClassLoader produced 
>> illegal classloaders.
>> Surefirebooter itself didn't check wheter it was forking, and just 
>> used the system classloader,
>> even if it wasn't forking. Ofcourse, when it uses the system 
>> classloader, it can't add the testcases.
>> The usesystemclassloader basically means 'don't construct a 
>> classloader for the tests, they
>> are already present in the system classloader'.
>>
>> I refactored surefirebooter to include the usesystemclassloader in the 
>> forkConfiguration.
>> the 'isUseSystemClassLoader' method checks wheter forking is enabled 
>> in addittion to the useSCL flag.
>>
>> I think this fixes a lot of regressions.
>>
>> Note: by moving the flag to the fork configuration, i have to 
>> construct a new forkconfig in main()
>> otherwise it NPE's. I set the forkmode to never and the useSystemCL 
>> flag accordingly. In the runsuitesinprocess
>> only the forkconfig.useSystemCL is used.
>> Design-wise, the configuration used in the runSuitesInProcess methods 
>> should be different from the
>> global config and ideally the useSystemCL should be present globally, 
>> or wrapped in another class.
>> However, having 2 places to look for a flag is error prone. I know 
>> this design may not be the best
>> avail, but at least now there's no difference anymore between calling 
>> surefirebooter from main() or via run().
>>
>> I deployed new snapshots of surefire for people to test. I hope this 
>> fixes your regressions.
>>
>> -- Kenney

Re: useSystemClassLoader problems

[Brett Porter <br...@apache.org>]

Thanks Kenney!

Please make sure to merge that up to the 2.3.x branch - this will  
close out SUREFIRE-335.

Cheers,
Brett

On 04/06/2007, at 9:43 PM, Kenney Westerhof wrote:

>
>
> Kenney Westerhof wrote:
>
>> Brett Porter wrote:
>
> [snip]
>
>>> However, I'm rethinking setting it to default (which I did) -  
>>> seems best to retain it as false. I did it because of another  
>>> regression where clases in the system classloader weren't found  
>>> in tests by default any more. I think that's just an omission in  
>>> creating the isolated classloader that can be fixed differently.
>
> I'm setting it back to false now.
>
> I found the cause of the regression - due to the fact that the  
> default for useSystemClassloader was true,
> this bug was made evident.
>
> The combination of forkmode=never and useSystemClassLoader produced  
> illegal classloaders.
> Surefirebooter itself didn't check wheter it was forking, and just  
> used the system classloader,
> even if it wasn't forking. Ofcourse, when it uses the system  
> classloader, it can't add the testcases.
> The usesystemclassloader basically means 'don't construct a  
> classloader for the tests, they
> are already present in the system classloader'.
>
> I refactored surefirebooter to include the usesystemclassloader in  
> the forkConfiguration.
> the 'isUseSystemClassLoader' method checks wheter forking is  
> enabled in addittion to the useSCL flag.
>
> I think this fixes a lot of regressions.
>
> Note: by moving the flag to the fork configuration, i have to  
> construct a new forkconfig in main()
> otherwise it NPE's. I set the forkmode to never and the useSystemCL  
> flag accordingly. In the runsuitesinprocess
> only the forkconfig.useSystemCL is used.
> Design-wise, the configuration used in the runSuitesInProcess  
> methods should be different from the
> global config and ideally the useSystemCL should be present  
> globally, or wrapped in another class.
> However, having 2 places to look for a flag is error prone. I know  
> this design may not be the best
> avail, but at least now there's no difference anymore between  
> calling surefirebooter from main() or via run().
>
> I deployed new snapshots of surefire for people to test. I hope  
> this fixes your regressions.
>
> -- Kenney

Re: useSystemClassLoader problems

[Kenney Westerhof <ke...@apache.org>]

Kenney Westerhof wrote:

> Brett Porter wrote:

[snip]

>> However, I'm rethinking setting it to default (which I did) - seems 
>> best to retain it as false. I did it because of another regression 
>> where clases in the system classloader weren't found in tests by 
>> default any more. I think that's just an omission in creating the 
>> isolated classloader that can be fixed differently.

I'm setting it back to false now.

I found the cause of the regression - due to the fact that the default for useSystemClassloader was true,
this bug was made evident.

The combination of forkmode=never and useSystemClassLoader produced illegal classloaders.
Surefirebooter itself didn't check wheter it was forking, and just used the system classloader,
even if it wasn't forking. Ofcourse, when it uses the system classloader, it can't add the testcases.
The usesystemclassloader basically means 'don't construct a classloader for the tests, they
are already present in the system classloader'.

I refactored surefirebooter to include the usesystemclassloader in the forkConfiguration.
the 'isUseSystemClassLoader' method checks wheter forking is enabled in addittion to the useSCL flag.

I think this fixes a lot of regressions.

Note: by moving the flag to the fork configuration, i have to construct a new forkconfig in main()
otherwise it NPE's. I set the forkmode to never and the useSystemCL flag accordingly. In the runsuitesinprocess
only the forkconfig.useSystemCL is used.
Design-wise, the configuration used in the runSuitesInProcess methods should be different from the
global config and ideally the useSystemCL should be present globally, or wrapped in another class.
However, having 2 places to look for a flag is error prone. I know this design may not be the best
avail, but at least now there's no difference anymore between calling surefirebooter from main() or via run().

I deployed new snapshots of surefire for people to test. I hope this fixes your regressions.

-- Kenney

Re: useSystemClassLoader problems

[Kenney Westerhof <ke...@apache.org>]

Brett Porter wrote:
> Sorry, read these out of order with my other response :)
> 
> On 01/06/2007, at 2:29 AM, Kenney Westerhof wrote:
> 
>> I'll have to see what the issues are, but there are some usecases for 
>> flat classloading
>> apps, where surefire needs everything in the system classloader. I'm 
>> sure you understand
>> the need for this feature - i've tried to convince people otherwise 
>> but finally saw that
>> it was a valid usecase.
> 
> Ok, makes sense. It's more consistent with how IDEs run it too.
> 
> However, I'm rethinking setting it to default (which I did) - seems best 
> to retain it as false. I did it because of another regression where 
> clases in the system classloader weren't found in tests by default any 
> more. I think that's just an omission in creating the isolated 
> classloader that can be fixed differently.

Hm, that shouldn't have anything to do with this approach. Surefire and
the test classes are (IIRC) all in the system classloader - at least according
to the manifest. I think there are no extra classloaders created when running
with useSystemClassLoader=true; at least that should be the case.

The regressions may have to do with the issue I described in the other mail, that the
directories need to end in /; test-classes are typically located in target/test-classes
so this may be the issue there.

Anyway, I think useSystemClassLoader should be false by default, as it's not really safe. 
See below.

>>> On 25/05/2007, at 5:01 PM, Brett Porter wrote:
>>>> I'm starting to find a number of bugs related to 
>>>> useSystemClassLoader, by virtue of it being a forked JAR call. I'd 
>>>> be interested to get more input on why this route was taken and how 
>>>> the classloading in general works now (would be a handy document to 
>>>> have).
>>
>> See above for the reason of this solution.
>>
>> Which bugs exactly?
> 
> There were a couple I fixed up already (not working on windows because 
> of the urls in the manifest, seem to be fine now which is the urlutils 
> thing I used - hope it still works for you). There are 334 (easy to fix) 
> and 335 (doesn't look critical).

334: indeed, if you run all in the system classloader, surefire and it's dependencies
are present there too. There's no real easy way around this. I can think of two solutions:

1) reorder the manifest classpath to put surefire last, perhaps the booter first. This may
   introduce conflicts when running this on surefire itself, or when the tests include a dep
   on anything surefire uses. Since 2 classpaths are merged, there are duplicate entries. If you
   filter them out, which one should you take - the one surefire mentions, so surefire works, but the
   tests are invalid, or the one the project declares, where surefire may no longer work.

2) Add a single class without dependencies to the test classpath as the Main (so not surefire-booter),
   that reads the surefire*.tmp files and constructs a new classloader (child of system), containing
   surefire and surefirebooter.
   Also put no surefire deps in the test classpath. Either create a new project (with the single class and no deps)
   and add that as a dep to the jar classpath, or copy the class into the test-classes directory (from
   the surefire-booter project, using getResourceAsstream("org/apache/maven/surefire/Main.class") or something.

I think the 2nd option is best - the jar itself should not contain any surefire classes, except the main one.

wdyt? We can also just say 'dont use this except you're dealing with legacy apps that don't understand classloading',
which rules out all maven code. The usesystemclasspath

issue 335: must be the '/' bug i fixed (which was not ported to trunk).
Do you have a testcase; can you reproduce this?

> I'd definitely be interested in getting together a doc on how the 
> classloading works though. I can certainly do it by investigation, but 
> if you have any thoughts you could jot down that'd be great :)

Sure, but it may require some more changes..

The idea is that there are 2 classloaders; the parent containing the tests, the child containing surefire,
as to avoid polution. The child should have 'childFirst' set to true (so the surefire loader is consulted first,
then the test classloader. 
For testing surefire itself, you'll have the same setup and it should work fine, as the tests themselves
are only present in the parent classloader. When those tests load surefire classes, they come from the parent,
so the correct classes are tested. The classes in the surefire child classloader are invisible to the parent
and will not pollute the parent. That's the idea.

There are currently 3 operating modes:
- forkmode none
    parent loader: isolated (no parent, not even system) classloader with tests
    child loader: parent=parent loader, configured by plugin/booter, containing surefire.
- forkmode once/pertest, usesystemclassloader=false
  have to check the code for this one, but i think it's something like this:
    system classloader: contains surefire booter+deps
    parent loader: isolated (no parent) loader with test classes
    child loader: parent=parent loader, contains surefire provider (junit etc)
- forkmode once/pertest, usesystemcl=true
    system classloader: contains tests and surefire booter + deps:
       surefire-booter
       surefire-api
       plexus-utils
       commons-lang
       plexus-archiver
    child loader: parent is system loader, contains surefire provider+deps

That's how it is right now, but the code may be different atm - it's been a while since I worked on this.

If you have any more questions or need some help, don't hesitate to ask.

Cheers,

   Kenney

>>>>> Also, can you check whether 
>>>>> http://jira.codehaus.org/browse/SUREFIRE-117?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel 
>>>>> should be closed as fixed as it was originally, and a new issue 
>>>>> created for their problems, or if it wasn't in fact fixed?
>>
>> It was fixed; i asked for feedback there. No idea why it doesn't work 
>> for them.
>> I do however know there were issues with the surefire plugin, but 
>> haven't seen those problems since.
> 
> Cool, I'll take another look.
> 
> Cheers,
> Brett