gnodet commented on pull request #690: URL: https://github.com/apache/maven/pull/690#issuecomment-1064222692
> > It _was_ leaking : the current PR still reverts at the end of the build. > > I don't think this is enough... as mentioned in the change, there is even this classloader used later on an further leaked as "the original" one as it is passed to another method call. > > > I agree that specifying build extensions at various level of a reactor may lead o unpredictable results > > I think this must be supported without issues, just think about a aggregator build pom that includes different other projects then some might define an extension an other don't. That will lead to failures in this build but not in the individual build. That's really a nightmare to debug and that why I'm a bit strict at those rules to not leaking classloaders. If at some places it is necessary that the project classloader is used it has to be set but reset instantly afterwards! > > > Actually, the one that cause problems is the [BuildCacheMojosExecutionStrategy](https://github.com/apache/maven-build-cache-extension/blob/master/src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java), and yes, it's session scoped because I'm aiming at integrating this extension in `mvnd` which reuses maven plexus realms and thus makes a much bigger difference between `@Singleton` and `@SessionScoped` components. > > But it seems you are rely on undefined behavior here then and what you wantt to archive could actually only work with a core-extension. My assumption so far was that build extensions defined in the root pom should be available to all modules built within the reactor. This implies that the classloader will not be destroyed before the session ends anyway and beans available if needed. This is broken, so I consider the previous commit a regression. The consequence is that the fact that the classloader is still the TCCL when building children is just a needed consequence of the above. IIUC, the leak originally mentioned in MNG-7402 was that the classloader was *never* restored. This PR actually restores the original TCCL after the build, so I think the original use case / problem is covered. That said, I'm all for improving the way extension works. I've raised https://github.com/apache/maven/pull/616 a while ago, though this is about core extensions, but I think both have valid use cases. But the use case you mention with aggregators for various projects is valid and would have to be covered, but I think it's broken now (especially with concurrent builds), so I'd rather address such new use cases in a different JIRA/PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
