On Mon, Feb 8, 2010 at 2:32 AM, Brett Porter <br...@apache.org> wrote:
>
> On 07/02/2010, at 6:16 AM, Kristian Rosenvold wrote:
>
> > I just discovered that the source of the hideous concurrency problem
> > I've been tracking for some time is the system property "user.dir".
>
>
> > Surefire basically sets the following three system properties:
> >
> > "basedir" = basedir.getAbsolutePath()
> > "user.dir" = workingDirectory.getAbsolutePath();
> > "localRepository" = localRepository.getBasedir();
>
I may be wrong, it may have been any of the three system properties.
Commenting
them out in surefire "solved" my problem, and it does not seem to have any
immediate
effect on anything (there is a SUREFIRE-419 attached to one of the
properties so I'll
see if that's still relevant).
> >
> > These properties are also used inside maven core, and creates
> > some interesting concurrency problems because by the time surefire sets
> > these values they may have changed already ;)
>
> Where are they used in the core? I thought it had been removed and would
> only be accessed on the initial set up from the cli.
>
PluginParameterExpressionEvaluator (line 89) references "user.dir", which
was
what had me immediately suspecting that property. But I am unsure if this
block
of code is "live"; there are unit tests using it but upon real execution it
doesn't
get executed. I just compiled the following code (skipTests=true on m3
itself, since
it's being used by a single unit test)
if ( basedir == null )
{
throw new RuntimeException("user.dir not in use");
//basedir = System.getProperty( "user.dir" );
}
I ran the full set of integration tests and the surefire tests and they all
passed, so I think this
code is dead.
It seems like this forks into 2 different problems:
A) Why/what/where is core picking up the system properties
B) Why is surefire setting them.
My immediate interest is really B. I see that the setting of the system
properties were
introduced in r382683 and r606809. Grepping for "System.setProperty" within
surefire
reveals that surefire is being very eager to manipulate local environment
to run in-process, there's more cases SureFireBooter.java line
330<http://svn.apache.org/viewvc/maven/surefire/trunk/surefire-booter/src/main/java/org/apache/maven/surefire/booter/SurefireBooter.java?view=markup&pathrev=HEAD>
It feels to me like the "correct" thing to do is not to manipulate the
runtime-environment
with forkmode=never, in other words stop supporting /any/ kind of
System.setProperty
for non-forked operation. This would break backwards compatibility, which I
understand is
not too popular. An alternate solution would be to disallow forkmode=never
within concurrent
running (or just upgrade to forkmode=once with a nice message)
> I wouldn't be opposed to Surefire not supporting setting system properties
> in non-forking mode in a future release, it seems like a recipe for disaster
> in an embedded / concurrent environment. I think it was done to encourage
> some consistency between the forked and non-forked versions.
>
I'm basically feature-complete with the m3 core for weave mode, and now it's
all down to
bug-hunting in the actual end-user experience; mvn install.
War-plugin and surefire will be needing patches, and I suppose I'll make
jira issues
and fix these once I get a stable version running 24/7. I think there's
little point in
committing any code "live" before 24/7 stability can be demonstrated for a
decent-sized
group of projects.
Kristian
