laeubi commented on pull request #690: URL: https://github.com/apache/maven/pull/690#issuecomment-1065058784
> 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. Alright. > 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. If you want to have "project-visibility" use `BuilderCommon.attachToThread( session.getCurrentProject() );` (but please reset the TCCL afterwards ;-) > I think we want the maven classloader, core extensions and build extensions to be used to lookup the beans. I agree about the first two but not the last one see `BuilderCommon.attachToThread` or alike should give you the desired effect. > 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. I also noticed that, but please also note the TODO (that was there before) that the author of that code (haven't diged in here) is sure if it is actually necessary, so probably it would be better to not call `BuilderCommon.attachToThread` at all here. > Which means that the purpose was to actually set the classloader to the correct value The purpose is to set the Thread loader to the one of the project while calling `projectBuilds.add( new ProjectSegment( project, taskSegment, copiedSession ) );` but even the author was not sure if it is actually incorrect... > 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... It is set back _after_ the build, but _while_ the build it leaks a (random) classloader and makes the project realm accessible to a number of (random) other items as you have proven with your extension. Please also take a look at where it is called in line 105 we have the "real" context-classloader (also note that the context clasloader might be different from the classlaoder used to actually load the current class!) https://github.com/apache/maven/blob/97c1e4b4aa73f5851801fdf5e17f013dd46a6b3e/maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecycleStarter.java#L105-L127 in line 122 the the current CCL (**what is now is a random leaked from the previous call**) is fetched and further passed to another component as the `originalContextClassLoader` (even though called on the call site `oldClassloader`) and this value **is** actually used later on to restore the CCL **after** a `BuilderCommon.attachToThread` is called in https://github.com/apache/maven/blob/97c1e4b4aa73f5851801fdf5e17f013dd46a6b3e/maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecycleModuleBuilder.java#L107-L158 just assume your extension is there and works, now I add another extension (e.g. a wagon-provider) and your extension suddenly fails with CNF or CCE (thats what my fix reveals without another extension being used), would you really desire/expect this? -- 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
