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.
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. 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]
