On 8 February 2010 09:18, Kristian Rosenvold <[email protected]>wrote:
> On Mon, Feb 8, 2010 at 2:32 AM, Brett Porter <[email protected]> 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 don't think it is acceptable to tweak an explicitly set parameter on a mojo just to "fix" the user's build (also know as doing a "Hudson m2 project type"). the default forkmode is once, they must have some good reason for setting it to never (like they want to run out of permgen space, etc) ;-) I think it would be acceptable to break a build if forkmode=never and parallel build=true... i.e. have the maven api's expose the fact that we are running in a parallel build, and then surefire can bomb the build out saying that forkmode=never is incompatible with parallel builds... then the user can decide to switch back to forkmode=once, or not use parallel builds... I think that there are a number of other plugins which might want to be aware of a parallel build... e.g. maven-invoker-plugin, jetty-maven-plugin, etc... so I do think that the maven 3 API's need to have some mechanism of exposing to plugins that they are running in a parallel build.... how we add this information in such a way that plugins will still work for 2.x as well as 3.x is an interesting question -Stephen > > > > 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 >
