gnodet commented on pull request #690: URL: https://github.com/apache/maven/pull/690#issuecomment-1065043792
For the record, the lookup of the `MojosExecutionStrategy` which was introduced by https://github.com/apache/maven/commit/1954d51ff2841045b4af8a515ad0719805269d8d is definitely wrong and the lookup should be either performed explicitely using the container or better with an injected `Provider` as explained in [Guice](https://github.com/google/guice/wiki/InjectingProviders#providers-for-mixing-scopes). It was developed explicitely for `m-build-cache-e` so it's meant to support looking up a session scoped bean, but the implicit injection done from the singleton scope to the session scope is definitely wrong. Which also raise the question about why is it allowed at all... but let's not diverge. This broken lookup will cause a problem when reusing the `MojoExecutor` which is a singleton and it would not lookup a new component if called again with in different session scope. I'll fix it by injecting a `Provider<MojosExecutionStrategy>` in the constructor and will do the actual lookup in the method when using the component. So let's consider the scoping problem solved please. The problem here is a *visibility* problem across realms. Even with the lookup fixed, the code without the current PR will still have no visibility on the required bean which is registered as an extension on the project which is being built. When the simple lookup is done (i.e. a `container.lookup()` or a `provider.get()` call), I think we want the maven classloader, core extensions and build extensions to be used to lookup the beans. So this has to be set up, and that's not the case. In order to have those lookups working correctly, we need the correct class realm to be setup. The last project defining extension was used and it was leaking after the build. With the current PR, the top level project one is used and does not leak after the build. Looking at the code leads me to think that the fix from https://github.com/apache/maven/commit/e327be3d85918a23a5ba48d752143a6dbf8b83f7 is wrong, because it removes the visibility to the build extensions. I back this point by the fact that the code now between the `attachToThread` call and the restoration of the TCCL does not seem to actually perform any lookup or class loading. Which means that the purpose was to actually set the classloader to the correct value, which is what I'm trying to get back. One thing I don't really get in what you say : why do you consider that this PR does not fix the classloader leak ? The classloader is set to the extension class loader if one is provided and back to its original value after the build, so that looks ok to me... -- 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
