[MNG-7429] The classloader containing build extension should be used throughout the build

[gnodet]

This fixes problems when using build extensions.

[michael-o]

This does not supersede e327be3, but complements it?

[michael-o]

@laeubi can you check also?

[gnodet]

This does not supersede e327be3 but complements it?

Yes, I actually don't think the first commit is really relevant or useful with the current fix. I think in most cases, only the top level project has a classloader, which means the previous commit won't actually do anything and the classloader will remain the same.

The problem is that I don’t think the behavior is defined for the “scope” of such extensions for reactor builds. My assumption is that they are defined on the top level project and should be available for the whole execution, hence the fix that registers the top level project classloader… a bit earlier in the maven execution.

So, I guess the answer should be yes !

[michael-o]

What is a build extension is declared in a submodule only?

[gnodet]

What is a build extension is declared in a submodule only?

You mean "what if" ?
That's my point, I don't think there are solid expectations wrt to the reactor hierarchy. What if it's defined in a child which itself has children ? etc....

[michael-o]

What is a build extension is declared in a submodule only?

You mean "what if" ? That's my point, I don't think there are solid expectations wrt to the reactor hierarchy. What if it's defined in a child which itself has children ? etc....

Yes, that was my question. Just a typo. So you don't expect any problems with that?

[gnodet]

What is a build extension is declared in a submodule only?

You mean "what if" ? That's my point, I don't think there are solid expectations wrt to the reactor hierarchy. What if it's defined in a child which itself has children ? etc....

Yes, that was my question. Just a typo. So you don't expect any problems with that?

I do expect problems, but I don't think it really matters because I don't think there's an actual use case for that.
I'll push another commit to revert the previous fix at the same time, so that the behavior will be the same as before (broken or not).

[laeubi]

This looks like another issue that is only similar to the other fix, but why should anything be reverted?

[gnodet]

This looks like another issue that is only similar to the other fix, but why should anything be reverted?

It's not another issue. The e327be3 commit actually breaks build extension. This PR fixes the problem, but the older commit is meaningless once this one is applied, as the classloader is already reverted by this fix (which was the original problem iiuc).

[laeubi]

But why/how does it breaks them? As described in the PR this currently leaks the CCL of the last project in the chain, and even if it makes extensions work somehow it is clearly broken so I think both are probably needed...

[gnodet]

But why/how does it breaks them? As described in the PR this currently leaks the CCL of the last project in the chain, and even if it makes extensions work somehow it is clearly broken.

See apache/maven-build-cache-extension#8 where the build extension test is broken on 3.9.x. I bisected the problem to e327be3.

The reason is that the extension defines a component with a session scope. This component is defined in the project's classloader (because it's a build extension and not a core extension). This means that when the component is loaded, the projects classloader has to be used, else the component will not be seen by plexus. The previous commit reverts the TCCL to the core maven one before actually building the project. This PR aims at broadening the window where the project's classloader is used, and still revert it at the end.

Also, about the last project point, I think in most cases, only the top level project has a specific classloader. The behavior before the previous commit was then that the top level project was used to set the TCCL, and the other projects, not having any specific classloader defined (if not defining any build extension), were simply ignored.

[michael-o]

@gnodet Does this apply to 3.8.x? I guess we need a new JIRA issue to clearly document this problem/regression.

[laeubi]

But this does actually shows that an project classlaoder is leaked. I don't see how reverting this makes the situation better (beside that 'it has worked before') as obviously here we have a gap where a random project classloader is used.

[laeubi]

Also, about the last project point, I think in most cases, only the top level project has a specific classloader.

If it is not the last then "the last one that has a specific classloader", and working of such extensions then was jsut a side-effect and if thats true what you wrote why then all these "attach to thread" for each project? As far as i understand the "top level" is the pom actually executed and assume that that is the only valid one to have extensions seems a bit strange.

[laeubi]

The reason is that the extension defines a component with a session scope.

By the way I already noticed that SessionScoped components are actually hard to implement, I used the LegacySupport as the only workaround to access the session in a maven-extension.

[laeubi]

Do you refer to https://github.com/apache/maven-build-cache-extension/blob/master/src/main/java/org/apache/maven/buildcache/CacheLifecycleParticipant.java ? Why do you make this a session scoped @Named and not a plain @Component(role = AbstractMavenLifecycleParticipant.class, hint = "cache")? Thats how tycho works and maven sets the appropriate classlaoder when calling the methods.

[gnodet]

@gnodet Does this apply to 3.8.x? I guess we need a new JIRA issue to clearly document this problem/regression.

@michael-o It does apply to 3.8.x, and yes, I'll create a JIRA.

But this does actually shows that an project classlaoder is leaked. I don't see how reverting this makes the situation better (beside that 'it has worked before') as obviously here we have a gap where a random project classloader is used.

It was leaking : the current PR still reverts at the end of the build. And yes, I agree that specifying build extensions at various level of a reactor may lead o unpredictable results. However,

• I think the current behavior (i.e. an extension defined at the top should be effective throughout the build) should be preserved
• while defining extensions inside the reactor is theoretically possible, I don't think any used that and I don't think we need to support those use cases
  This leads me to think that reverting to the previous behavior is the way to do, while still avoiding the leak at the end of the build.

Also, about the last project point, I think in most cases, only the top level project has a specific classloader.

If it is not the last then "the last one that has a specific classloader", and working of such extensions then was jsut a side-effect and if thats true what you wrote why then all these "attach to thread" for each project? As far as i understand the "top level" is the pom actually executed and assume that that is the only valid one to have extensions seems a bit strange.

Defining extensions not at the top level was broken, as you said, the last project defining extensions was used for the whole build. So I assume that's actually not a use case and we don't need to fix it. It would be easier to forbid them at the moment if we really want to fix the possible problem.

Do you refer to https://github.com/apache/maven-build-cache-extension/blob/master/src/main/java/org/apache/maven/buildcache/CacheLifecycleParticipant.java ? Why do you make this a session scoped @Named and not a plain @Component(role = AbstractMavenLifecycleParticipant.class, hint = "cache")? Thats how tycho works and maven sets the appropriate classlaoder when calling the methods.

Actually, the one that cause problems is the BuildCacheMojosExecutionStrategy, 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.

[laeubi]

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

[gnodet]

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, 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 #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.

[gnodet]

I've added a commit to revert e327be3 because it's useless.

This restores the behavior from 3.8.4 when extensions are defined in the root pom and allows supporting the maven-build-cache-extension use case at apache/maven-build-cache-extension#8. This would break builds that would define extensions in the parent and in a child and expect them to be available later in the build. If we want to completely restore the 3.8.4 behavior, I can remove line https://github.com/apache/maven/pull/690/files#diff-7698873d65eece16fdf2ba67293827f623994a3affa509b814eaf33abb4537daR84 so that the last project with extensions wins.

I agree this is not perfect and we should better support defining build extensions throughout the various projects in the reactor. Such use cases are clearly not well covered, especially inheritance between parent/child projects, ordering, concurrency, etc...

@michael-o @laeubi I'm willing to write an IT, but I can't write until we agree on the behavior...

[laeubi]

I've added a commit to revert e327be3 because it's useless.

It is not useless but prevents a classloader leak as you have proven.

This restores the behavior from 3.8.4 when extensions are defined in the root pom

From the code it is completely irrelevant where you define your extension as long as you only define exactly one and this is simply a wrong assumption.

I'm willing to write an IT

I don't think it is good to make such behavior part of the covered assumptions. Whether or not an extension is defined in the root or not should be completely opaque and all code that works with extensions seems to not assume that but that project defined defined extensions are project scoped (taken the usual merging rules into account).

but I can't write until we agree on the behavior...

I think instead of reverting / restoring wrong behavior it would be much more profitable to find out whats the root cause for this. For example a stack trace where a CNF exception is triggered and maybe then we see that there is actually a attatchToThread is missing somewhere else (or we have other classloader leaks that are revealed by the fixed behavior).

Such use cases are clearly not well covered, especially inheritance between parent/child projects, ordering, concurrency, etc...

Just from reading the code and debugging the maven internals, a session-scoped component was never meant to be provided by a project-scoped extension and that it worked was just a side-effect but I could be wrong. At laest the Maven-CLI has distinct Classlaodersetups on the cases where a project scoped extension has to be accessed.

[laeubi]

Just one thing I'm curious about: Why is the mavencache not used as a core-extension? I would suspect that this will give much more powerful and stable integration than just integrate it on the project level.

[laeubi]

Actually, the one that cause problems is the BuildCacheMojosExecutionStrategy,

Does this actually has to be session scoped at all? As the session is passed in is it really session scoped? Is the problem in the class itself or is the class not discovered? Because MojoExecutor is a singelton I don't think it actually is valid to assume it could use anything session/project scoped. If that is to be supported, I thin the MojosExecutionStrategy should not be injected but locked up for each project in MojoExecutor.execute(MavenSession, List<MojoExecution>, ProjectIndex)

[laeubi]

Just another observation, it seems MojosExecutionStrategy was just intorduced in maven 3.9.x so how could it be a regression from 3.8.x?

[laeubi]

Just for references as I recently added support for project-scoped WorkspaceReaders, if you like to support such components one needs to add explicit support for them similar to:

maven/maven-core/src/main/java/org/apache/maven/DefaultMaven.java

Lines 371 to 379 in 97c1e4b

	for ( WorkspaceReader workspaceReader : getProjectScopedExtensionComponents( session.getProjects(),
	WorkspaceReader.class ) )
	{
	if ( workspaceReaders.contains( workspaceReader ) )
	{
	continue;
	}
	workspaceReaders.add( workspaceReader );
	}

maven/maven-core/src/main/java/org/apache/maven/DefaultMaven.java

Lines 507 to 542 in 97c1e4b

	protected <T> Collection<T> getProjectScopedExtensionComponents( Collection<MavenProject> projects, Class<T> role )
	{
	Collection<T> foundComponents = new LinkedHashSet<>();
	Collection<ClassLoader> scannedRealms = new HashSet<>();
	Thread currentThread = Thread.currentThread();
	ClassLoader originalContextClassLoader = currentThread.getContextClassLoader();
	try
	{
	for ( MavenProject project : projects )
	{
	ClassLoader projectRealm = project.getClassRealm();
	if ( projectRealm != null && scannedRealms.add( projectRealm ) )
	{
	currentThread.setContextClassLoader( projectRealm );
	try
	{
	foundComponents.addAll( container.lookupList( role ) );
	}
	catch ( ComponentLookupException e )
	{
	// this is just silly, lookupList should return an empty list!
	logger.warn( "Failed to lookup " + role + ": " + e.getMessage() );
	}
	}
	}
	return foundComponents;
	}
	finally
	{
	currentThread.setContextClassLoader( originalContextClassLoader );
	}
	}

as you see, the calling code should also be aware that there are multiple items and has to handle this (combine, call in a row, throw exception ...).

[gnodet]

Just for references as I recently added support for project-scoped WorkspaceReaders, if you like to support such components one needs to add explicit support for them similar to:

[...]

as you see, the calling code should also be aware that there are multiple items and has to handle this (combine, call in a row, throw exception ...).

I tend to think that the current design is somewhat broken if whenever you need to lookup a component, you need to go through a complex setup in order to find where to load the component from.
The plexus container has support for importing from various visible realms. I think at each stage of the build (after entering the session scope, during the build of a given project, during the execution of a mojo), a corresponding realm should be setup and associated to the TCCL so that lookups can be performed with the correct visibility and scope. Failing to setup such a realm with result in an inability to do loops correctly : as you explained for looking up a list, but for a single component, you'd have to deal with priority, ordering and all concerns that should be left to the container.

but other think defining extensions in the pom is preferable

Just in case you don't know it (and to save you from to much hassle) it is no longer required to define core-extensions in lib/ext you can simply define them in .mvn/extensions.xml of the project, and I think that's what you actually try to arcive by your "master-project-classloader" see here for the documentation: https://maven.apache.org/ref/3.8.4/maven-embedder/core-extensions.html

You can take a look here for an example of the tycho-build extension that is a core-extension: https://github.com/eclipse/tycho/tree/master/tycho-its/projects/reactor.makeBehaviour

Thx, I did set up the tests for the m-build-cache-e using .mvn/extensions.xml so that's definitely not an issue.

[laeubi]

I tend to think that the current design is somewhat broken if whenever you need to lookup a component, you need to go through a complex setup in order to find where to load the component from.

You don't need it "whenever you lookup" you only need it for the specific cases of where you have a project-scoped extension and that is required to isolate the class-realms to not leak classes from one project into another.

The plexus container has support for importing from various visible realms. I think at each stage of the build (after entering the session scope, during the build of a given project, during the execution of a mojo), a corresponding realm should be setup and associated to the TCCL so that lookups can be performed with the correct visibility and scope.

Sure why not, but then your extension would still not work in a session-scoped nor a singelton created in "root scope"... So your usage has only worked due to a bug (as far as I can tell from the provided information here) and I don't see how it could work with other ways of class-loader isolation.

so that's definitely not an issue.

So I'm really confused what is the issue then? As from the code, your problematic component is to be used in a singleton injected and called, how is it (and why) supposed to work if it is defined in a project scope? extensions.xml is exactly the place to place such "global for all" plugins, the pom.xml is not ... If one likes to "break" the rules then additional setup is required, unless maven supports better ways, e.g. @ProjectScoped but then still someone has to setup this scope at distinct points (e.g. wen executing the mojos of a project and can't assume it works everywhere... so most probably MojoExecutor has to not reference the MojoExecutionStrategy in the constructor but look it up in the actual call.

[gnodet]

For the record, the lookup of the MojosExecutionStrategy which was introduced by 1954d51 is definitely wrong and the lookup should be either performed explicitely using the container or better with an injected Provider as explained in Guice. 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 e327be3 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...

[laeubi]

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" when fetching the Provider<MojosExecutionStrategy> use BuilderCommon.attachToThread( session.getCurrentProject() ); there (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!)

maven/maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecycleStarter.java

Lines 105 to 127 in 97c1e4b

	projectBuilds = buildListCalculator.calculateProjectBuilds( session, taskSegments );
	if ( projectBuilds.isEmpty() )
	{
	throw new NoGoalSpecifiedException( "No goals have been specified for this build."
	+ " You must specify a valid lifecycle phase or a goal in the format <plugin-prefix>:<goal> or"
	+ " <plugin-group-id>:<plugin-artifact-id>[:<plugin-version>]:<goal>."
	+ " Available lifecycle phases are: " + defaultLifeCycles.getLifecyclePhaseList() + "." );
	}
	ProjectIndex projectIndex = new ProjectIndex( session.getProjects() );
	if ( logger.isDebugEnabled() )
	{
	lifecycleDebugLogger.debugReactorPlan( projectBuilds );
	}
	ClassLoader oldContextClassLoader = Thread.currentThread().getContextClassLoader();
	ReactorBuildStatus reactorBuildStatus = new ReactorBuildStatus( session.getProjectDependencyGraph() );
	reactorContext =
	new ReactorContext( result, projectIndex, oldContextClassLoader, reactorBuildStatus );
	String builderId = session.getRequest().getBuilderId();

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

maven/maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecycleModuleBuilder.java

Lines 107 to 158 in 97c1e4b

	BuilderCommon.attachToThread( currentProject );
	projectExecutionListener.beforeProjectExecution( new ProjectExecutionEvent( session, currentProject ) );
	eventCatapult.fire( ExecutionEvent.Type.ProjectStarted, session, null );
	MavenExecutionPlan executionPlan =
	builderCommon.resolveBuildPlan( session, currentProject, taskSegment, new HashSet<>() );
	List<MojoExecution> mojoExecutions = executionPlan.getMojoExecutions();
	projectExecutionListener.beforeProjectLifecycleExecution( new ProjectExecutionEvent( session,
	currentProject,
	mojoExecutions ) );
	mojoExecutor.execute( session, mojoExecutions, reactorContext.getProjectIndex() );
	long buildEndTime = System.currentTimeMillis();
	projectExecutionListener.afterProjectExecutionSuccess( new ProjectExecutionEvent( session, currentProject,
	mojoExecutions ) );
	reactorContext.getResult().addBuildSummary( new BuildSuccess( currentProject,
	buildEndTime - buildStartTime ) );
	eventCatapult.fire( ExecutionEvent.Type.ProjectSucceeded, session, null );
	}
	catch ( Throwable t )
	{
	builderCommon.handleBuildError( reactorContext, rootSession, session, currentProject, t, buildStartTime );
	projectExecutionListener.afterProjectExecutionFailure( new ProjectExecutionEvent( session, currentProject,
	t ) );
	// rethrow original errors and runtime exceptions
	if ( t instanceof RuntimeException )
	{
	throw (RuntimeException) t;
	}
	if ( t instanceof Error )
	{
	throw (Error) t;
	}
	}
	finally
	{
	if ( scoped )
	{
	sessionScope.exit();
	}
	session.setCurrentProject( null );
	Thread.currentThread().setContextClassLoader( reactorContext.getOriginalContextClassLoader() );

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?

[laeubi]

For the record, the lookup of the MojosExecutionStrategy which was introduced by 1954d51 is definitely wrong and the lookup should be either performed explicitly using the container or better with an injected Provider as explained in Guice.

Also just for the record: You won't have noticed this bug without the fix applied in e327be3 and that's why I'm so eager to fix bad usage of CCL/leakage as these hides hard to track problems you can spend hours of debugging and banging your head on the wall, even though most of the time people don't feel its a problem or even don't think its worth a change see https://issues.apache.org/jira/browse/CAMEL-10456 for another nasty problem in that category.

[gnodet]

@laeubi I've pushed another commit to discuss / experiment with.

There are only two remaining calls to BuildCommon.attachToThread which are now clearly scoped in a block that grabs / restores the TCCL.
In short:

• the top level project's realm is used by default during the duration of the build
• if any project defines extensions, this realm is used instead when building that project
  Thoughts ?

[laeubi]

Thoughts ?

Looks like a good idea.

the top level project's realm is used by default during the duration of the build

I'm not sure why this should be the case... The "top level project" is not meant to be something special when it comes to classloading. Just assume the top-level is just an aggregator that includes all projects as modules (with the modules using individual parents) why should I want it to be used as the CCL root?

[gnodet]

Thoughts ?

Looks like a good idea.

the top level project's realm is used by default during the duration of the build

I'm not sure why this should be the case... The "top level project" is not meant to be something special when it comes to classloading. Just assume the top-level is just an aggregator that includes all projects as modules (with the modules using individual parents) why should I want it to be used as the CCL root?

I would assume some kind of implicit inheritance wrt to extensions. Another way would be to create a realm that would have visibility on all extensions of the build and use that one for the whole build duration. This would get rid of the attachToThread method completely.

[laeubi]

I would assume some kind of implicit inheritance wrt to extensions. Another way would be to create a realm that would have visibility on all extensions of the build and use that one for the whole build duration.

I don't think all extensions should share the same class-space, especially with projects-scoped extension (in contrast to core-extension) they started to become only active after a while, there is a bootstrap phase in maven when there is actually no such thing like "project", also as explained before you most probably don't want that an extension magically becomes active just because another module defines it. If you like that an extension applies for all projects then it should be defined as a core-extension in .mvn/extensions.xml so they become part of the bootstrap-process.

Thats also the reason why only core-extensions can participate in session-start because at that point there are no projects and thus any project-scoped extension is simply not accessible.

[gnodet]

I would assume some kind of implicit inheritance wrt to extensions. Another way would be to create a realm that would have visibility on all extensions of the build and use that one for the whole build duration.

I don't think all extensions should share the same class-space, especially with projects-scoped extension (in contrast to core-extension) they started to become only active after a while, there is a bootstrap phase in maven when there is actually no such thing like "project", also as explained before you most probably don't want that an extension magically becomes active just because another module defines it. If you like that an extension applies for all projects then it should be defined as a core-extension in .mvn/extensions.xml so they become part of the bootstrap-process.

Thats also the reason why only core-extensions can participate in session-start because at that point there are no projects and thus any project-scoped extension is simply not accessible.

Isn't that exactly the opposite of what is done for lifecycle participants and workspace readers at boot time ? Those are loaded from all registered extensions for all projects and are active for the whole build.

[laeubi]

Isn't that exactly the opposite of what is done for lifecycle participants and workspace readers at boot time ? Those are loaded from all registered extensions for all projects and are active for the whole build.

But each of those are loaded from their own classloader (there is no shared one) if they are defined at the project level. If they are defined as core-extension they are all loaded from the "maven-core-realm", that's why I said if you want to be on the "global-scope" (thus all projects are sharing the same instance) you should use a maven-core extension.

The whole caching-story for me sounds as if it is more suitable as a core-extension, for example tycho is defined on a per project level, but we check as the very first step that all projects in the reactor are using the same tycho version and fail the build if not. Neverless my goal is to make tycho a pure core-extension because project-scoped is much more limited and requires special care.

[gnodet]

Isn't that exactly the opposite of what is done for lifecycle participants and workspace readers at boot time ? Those are loaded from all registered extensions for all projects and are active for the whole build.

But each of those are loaded from their own classloader (there is no shared one) if they are defined at the project level. If they are defined as core-extension they are all loaded from the "maven-core-realm", that's why I said if you want to be on the "global-scope" (thus all projects are sharing the same instance) you should use a maven-core extension.

But I'm not talking about changing the class loaders, just the visibility for beans with plexus lookup. I don't see why WorkspaceReader and AbstractMavenLifecycleParticipant should be treated differently than any other lookup.

The whole caching-story for me sounds as if it is more suitable as a core-extension, for example tycho is defined on a per project level, but we check as the very first step that all projects in the reactor are using the same tycho version and fail the build if not. Neverless my goal is to make tycho a pure core-extension because project-scoped is much more limited and requires special care.

Core extensions are not loaded if you're aggregating multiple projects as you hinted earlier, so I guess it really depends on the use case. I don't really any good reason to not try to support both.

I've added a commit which explains my thoughts : a single ClassRealm is created with all extensions (and not only the ones from top level or the last project). This realm is not used to load classes from, it's only used for lookups. It's created as early as possible after the projects are created and used as the context classloader. It removes the need for explicit lookup in those realms for workspace readers and lifecycle participants.

[laeubi]

I'm not sure if I can give more input on this, but only quote this

it really depends on the use case

WorkspaceReader and AbstractMavenLifecycleParticipant are special because they are a mean of bootstrap and/or add additional look-ups on a global level, but at the same time they are very limited and require special care, e.g. as noted above depending on how an AbstractMavenLifecycleParticipant is provided not all methods are called and WorkspaceReader for example has no access to any project or session context.
Anyways I don't think one could assume that one always wan't to lookup everything from all project extensions, similar to when I define a mirror in a pom.xml I for sure don't want it to be used in other (unrelated) modules just because they are part of the same reactor, if I want this then I define it on the settings.xml ...

So for your case, if you really thing your extension should be defined on the project level, then for me it makes totally sense that this is only available in the projects where it is defined (either explicit or implicit e.g. though parent reference) and then your extension would not be looked up globally. If you want ti to act globally independent of defined in a project and you want to have much more control I think it should be a core-extension even if others not so involved in the details think "it might be useful" to define it on the project level.

@michael-o maybe you want to take over here or suggest others more familiar with how maven is supposed to work here.

[gnodet]

I'm not sure if I can give more input on this, but only quote this

it really depends on the use case

WorkspaceReader and AbstractMavenLifecycleParticipant are special because they are a mean of bootstrap and/or add additional look-ups on a global level, but at the same time they are very limited and require special care, e.g. as noted above depending on how an AbstractMavenLifecycleParticipant is provided not all methods are called and WorkspaceReader for example has no access to any project or session context. Anyways I don't think one could assume that one always wan't to lookup everything from all project extensions, similar to when I define a mirror in a pom.xml I for sure don't want it to be used in other (unrelated) modules just because they are part of the same reactor, if I want this then I define it on the settings.xml ...

Well, I'd like to be sure about that. All the use cases I've seen so far rather indicate extensions are global to the build. The primary use case was for wagon providers afaik and those are global. Also given, they were currently mostly global (i.e. the extensions classloader is set until another project also define extensions), I'd rather assume users expects them to be global. I'd rather go for a behavior than suits most use cases so far (lifecycle, workspace readers, wagon providers, build cache), rather than changing the behavior and have to rely on specific hacks for each kind of extension, it kinda defeat the purpose.

Do you have any actual use case of build extensions that should have a limited scope (which again, is not how they were working)... ?

So for your case, if you really thing your extension should be defined on the project level, then for me it makes totally sense that this is only available in the projects where it is defined (either explicit or implicit e.g. though parent reference) and then your extension would not be looked up globally. If you want ti to act globally independent of defined in a project and you want to have much more control I think it should be a core-extension even if others not so involved in the details think "it might be useful" to define it on the project level.

@michael-o maybe you want to take over here or suggest others more familiar with how maven is supposed to work here.

[michael-o]

@michael-o maybe you want to take over here or suggest others more familiar with how maven is supposed to work here.

Honestly, you both are much deeper in this issue, I very little clue about CL hierarchies. I will leave the decision upto to you both.

[laeubi]

Well, I'd like to be sure about that. All the use cases I've seen so far rather indicate extensions are global to the build.

Which ones? Just for the sake of leaving the field of theoretical discussions I have created an practical example that demonstrate that this is not the case, this contains:

• an aggregator pom that includes two modules (with-tycho-ext and without-tycho-ext)
• with-tycho-ext uses a custom packaging type of of the tycho-maven-plugin and declares the tycho extension
• without-tycho-ext uses a custom packaging type of of the tycho-maven-plugin and but does not declare the tycho extension

There are three combinations one can build those (as demonstrated here):

1. You build with-tycho-ext what succeeds (as extension is active)
2. You build without-tycho-ext what fails (as extension is not active)
3. You build the aggregator what fails as one of its projects is not configured with the tycho extension

This clearly demonstrate that extensions are not global to the build, and I can't imagine that "most users" would expect that the aggregator should magically work just because just because in one unrelated aggregated projects an extension is active.

The same applies to the caching extension as well, if I enable it for a one project in an aggregator I do not expect it to be active for all.

Also given, they were currently mostly global (i.e. the extensions classloader is set until another project also define extensions), I'd rather assume users expects them to be global.

I don't think users expect that we are leaking any item from one project to another if not explicitly decalred.

I'd rather go for a behavior than suits most use cases so far (lifecycle, workspace readers, wagon providers, build cache), rather than changing the behavior and have to rely on specific hacks for each kind of extension, it kinda defeat the purpose.

As mentioned before it depends on the case and there are fundamental difference in a caller of an extension decides that all project lifecycle listeners should be called, versus all project lifecycle are globally available (What they are not! You can't look them all up in a Mojo even though all are called in the setup phase).

Funny enough you have proven by your change that this instantly causes dreaded CCE:

Caused by: java.lang.ClassCastException: org.sonatype.plexus.components.cipher.DefaultPlexusCipher cannot be cast to org.sonatype.plexus.components.cipher.PlexusCipher

in integration tests, and just as of today I'm again facing a similar issue here for Tycho plugin, so I still think that classloader separation is more important than any vague user assumption.

But I don't see how we can proceed here, as a matter of fact neither lifecycle, workspace readers nor wagon providers seem to have any issue with that but only cache-extension and now you try to propose that all of those are actually wrong and cache-extension is the one to rule that out...

[gnodet]

I've tried a few things with your example and that led me to understand something I had missed. If you define the following in the root pom:

  <build>
    <extensions>
      <extension>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-maven-plugin</artifactId>
        <version>${tycho-version}</version>
      </extension>
    </extensions>
  </build>

then the build works correctly, which is exactly what I assumed would be working, but could not understand when reading the code and your comments. However I wrongly assumed the extension was using some kind of global scope mechanism, similar to the way workspace readers and lifecycle participants are scanned and registered. The mechanism is completely different.
In the above case, the extension is actually mapped to a plugin definition, and plugins are inherited. This means that defining an extension in the root pom is like defining the extension in all children (using the parent model, and the not aggregating project, even though they are often the same). Also, the DefaultMavenPluginManager uses a cache for realms, so that in those cases, a single classloader is reused for multiple usages of the extension. And last, beans which are @Singleton or @SessionScope are cached by the scope, thereby making the extension actually global.

In short, once I fix the bad lookup in maven, it seems the problem I had with the build cache extension does not occur anymore. Btw, if you could have a look at #692 and #693, that would be nice !

I'll rework my PR to not use the overall classloader I've introduced as a global one. And given it won't be a bug fix anymore, I'll target it for master only.

[laeubi]

. If you define the following in the root pom:

This works because the aggregator is an implicit parent.

[jira-importer]

Resolve #9031

[jira-importer]

Resolve #9167

[jira-importer]

Resolve #9031