On 12-07-27 8:48 AM, Kristian Rosenvold wrote:
I agree, DefaultClassRealmManager does just what it says; you asked
for a new instance you'll get one; no need to change that. The answer
to my original question seems to be that the funky logic is not there
for maven cli usage, but for IDE integrations. I'm not too happy about
this code being in core at all, hopefully there's at least a testcase
that will break if I remove the retry logic ;) But I dont use eclipse
so I'm leaving that behind.
I do not believe this is the case. Parallel execution and m2e are
similar when it comes to class realm management. In both cases it is
likely to have multiple instances of the same plugin coexist, so m2e
should be fine if you get rid of this logic without breaking anything
for parallel execution.
It looks to me like the test in
DefaultBuildPluginManager#getPluginRealm is just an optimization that
allows avoiding the lookup in
DefaultMavenPluginManager#pluginRealmCache. I could remove the whole
lookup and I'd just waste a few more cpu cycles building the cache key
and doing the lookup.
The heart of the problem seems to be some kind of race over the data
being used to create the cache key, I'm adding some logging of the
values in question to try to smoke it out. I'm not convinved
synchronizing getPluginRealm will help in this case.
getPluginRealm is lazy initializing PluginDescriptor.classRealm using
slightly disguised double-check-locking pattern [1]. I'd still
synchronize it, just to be safe.
I would not remove getPluginRealm, however. setupPluginRealm resets some
parts of PluginDescriptor, ComponentDescriptor.implementationClass in
particular, which will likely cause problems when multiple threads share
the same PluginDescriptor instance.
[1] http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
--
Regards,
Igor
Kristian
2012/7/27 Igor Fedorenko <[email protected]>:
Shouldn't DefaultBuildPluginManager#getPluginRealm be synchronized?
If two threads arrive at DefaultBuildPluginManager line 170 at about the
same time, they both will see null pluginRealm and both will then call
setupPluginRealm, which I believe will likely result in the two threads
using two different classloaders.
Also, I still do not believe there is anything wrong with
design/implementation of DefaultClassRealmManager. If it is asked to
createPluginRealm multiple times, it creates multiple realms. Doing
otherwise will result in the same realm being reused for dirrefent
project/plugin combos and will mess up classloading unless entire
classloading approach is reworked.
https://cwiki.apache.org/MAVEN/maven-3x-class-loading.html
--
Regards,
Igor
On 12-07-26 1:27 PM, Kristian Rosenvold wrote:
I have some very specific logs in an issue
(http://code.google.com/p/maven-svn-revision-number-plugin/issues/detail?id=16#c85)
indicating that to class realms are being created for the same plugin
when there is only one execution.
The behaviour seems totally consistent with hitting the catch block at
line 100 of DefaultClassRealmManager, down to
the suffix of the second class realm. Logically, this will happen at
any time when the FIRST construction of the class realm happens in two
parallel modules in the build. Imagine that you have two modules at
the end of the build, both initializing the war plugin. If they are
siblings in the parallel scheduling, the likelyhood of these two
requesting realm in parallel is extremely high.
The thread scheduling will ensure that two such modules will arrive at
DefaultBuildPluginManager line 170 at more or less the same instant.
Both will go through the if test, since this is the first time the
realm is created. The interesting thing is that it seems to me like
the cache key at line 304 must somehow be different, since the
setupPluginRealm method is synchronized.
It seems like I need to follow up on why the cache key is different,
because technically it looks like we should be avoiding the second
call to newRealm, meaning I should really be able to leave the funky
logic as-is.
WDYT ?
Kristian
2012/7/26 Igor Fedorenko <[email protected]>:
I have not looked at parallel execution closely and don't fully
understand the usecase, but I am not sure race condition is the code in
DefaultClassRealmManager.
As far as I can tell, DefaultClassRealmManager only provides create*ock
methods, which create new realms and the current implementation looks
proper.
If parallel execution logic needs to be able to create-or-query plugin
realms in thread-safe manner, I think this should be implemented
somewhere at higher levels, where one-plugin-realm-per-plugin-perproject
constraint can be enforced properly.
Again, I do not know parallel execution, so I can be completely wrong
about this.
--
Regards,
Igor
On 12-07-26 12:36 PM, Jason van Zyl wrote:
In that case we may just want to differentiate because the parallel
execution use case is not one that happens inside the IDE. In the case
of
CLI execution it can all be handled up front as additional realm
construction beyond the CLI execution isn't going to happen. In an IDE
or
shell wouldn't a map of realms keyed by GAV suffice? Sync on write
everyone
is free to read?
On Jul 26, 2012, at 12:06 PM, Igor Fedorenko wrote:
This is required in m2e where multiple versions of the same plugin can
coexist for long time in the same container.
--
Regards,
Igor
On 12-07-26 8:44 AM, Kristian Rosenvold wrote:
There is a race condition in parallel builds that occurs related to
this piece of code:
http://maven.apache.org/ref/3.0.4/maven-core/xref/org/apache/maven/classrealm/DefaultClassRealmManager.html#75
The thing is, for some reason, there's a loop that retries the class
realm generation with a random suffix if the class realm already
exists. In a parallel run, there will be multiple threads requesting
the same realm-id, which semantically should map to the same instance
of the class realm.
(Most plugins do not really mind if there's a duplicate class realm
every now and then, but some take it very seriously ;)
I'm tempted to change the semantics of the "newRealm" method to
"getOrCreateRealm", since that seems to be the correct semantics no
matter what. I've tried tracking the origin of the while loop, and it
seems to be very old. Anyone have any idea of what purpose it served ?
Kristian
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Thanks,
Jason
----------------------------------------------------------
Jason van Zyl
Founder & CTO, Sonatype
Founder, Apache Maven
http://twitter.com/jvanzyl
---------------------------------------------------------
The modern conservative is engaged in one of man's oldest exercises in
moral philosophy; that is,
the search for a superior moral justification for selfishness.
-- John Kenneth Galbraith
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
