You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/05/17 13:50:44 UTC
[GitHub] [maven] gnodet opened a new pull request, #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
gnodet opened a new pull request, #743:
URL: https://github.com/apache/maven/pull/743
https://issues.apache.org/jira/browse/MNG-7401
https://issues.apache.org/jira/browse/MNG-7474
@laeubi this supersedes https://github.com/apache/maven/pull/666
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1165776103
Is there a reasonable IT for 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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #743:
URL: https://github.com/apache/maven/pull/743#discussion_r912212544
##########
maven-core/src/test/java/org/apache/maven/lifecycle/internal/LifecycleModuleBuilderTest.java:
##########
@@ -0,0 +1,102 @@
+package org.apache.maven.lifecycle.internal;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.maven.execution.AbstractExecutionListener;
+import org.apache.maven.execution.DefaultMavenExecutionRequest;
+import org.apache.maven.execution.DefaultMavenExecutionResult;
+import org.apache.maven.execution.MavenExecutionRequest;
+import org.apache.maven.execution.MavenExecutionResult;
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.lifecycle.internal.builder.BuilderCommon;
+import org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder;
+import org.apache.maven.lifecycle.internal.stub.DefaultLifecyclesStub;
+import org.apache.maven.lifecycle.internal.stub.LifecycleTaskSegmentCalculatorStub;
+import org.apache.maven.lifecycle.internal.stub.LoggerStub;
+import org.apache.maven.lifecycle.internal.stub.MojoExecutorStub;
+import org.apache.maven.lifecycle.internal.stub.ProjectDependencyGraphStub;
+import org.apache.maven.plugin.MojoExecution;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.ContainerConfiguration;
+import org.codehaus.plexus.PlexusConstants;
+import org.codehaus.plexus.PlexusTestCase;
+import org.codehaus.plexus.logging.Logger;
+import org.junit.Test;
+
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+
+public class LifecycleModuleBuilderTest extends PlexusTestCase
+{
+ @Override
+ protected void customizeContainerConfiguration( ContainerConfiguration configuration )
+ {
+ configuration.setAutoWiring( true );
+ configuration.setClassPathScanning( PlexusConstants.SCANNING_INDEX );
+
+ }
+
+ public void testCurrentProject() throws Exception
+ {
+ List<MavenProject> currentProjects = new ArrayList<>();
+ MojoExecutorStub mojoExecutor = new MojoExecutorStub()
+ {
+ @Override
+ public void execute( MavenSession session, List<MojoExecution> mojoExecutions, ProjectIndex projectIndex )
+ throws LifecycleExecutionException
+ {
+ super.execute( session, mojoExecutions, projectIndex );
+ currentProjects.add( session.getCurrentProject() );
+ }
+ };
+
+ final DefaultMavenExecutionResult defaultMavenExecutionResult = new DefaultMavenExecutionResult();
+ MavenExecutionRequest mavenExecutionRequest = new DefaultMavenExecutionRequest();
+ mavenExecutionRequest.setExecutionListener( new AbstractExecutionListener() );
+ mavenExecutionRequest.setGoals( Arrays.asList( "clean" ) );
+ mavenExecutionRequest.setDegreeOfConcurrency( 1 );
+ final MavenSession session = new MavenSession( null, null, mavenExecutionRequest, defaultMavenExecutionResult );
+ final ProjectDependencyGraphStub dependencyGraphStub = new ProjectDependencyGraphStub();
+ session.setProjectDependencyGraph( dependencyGraphStub );
+ session.setProjects( dependencyGraphStub.getSortedProjects() );
+
+ LifecycleModuleBuilder moduleBuilder = lookup( LifecycleModuleBuilder.class );
+ set( moduleBuilder, "mojoExecutor", mojoExecutor );
+
+ LifecycleStarter ls = lookup( LifecycleStarter.class );
+ ls.execute( session );
+
+ assertNull( session.getCurrentProject() );
+ assertEquals( Arrays.asList( ProjectDependencyGraphStub.A, ProjectDependencyGraphStub.B, ProjectDependencyGraphStub.C,
+ ProjectDependencyGraphStub.X, ProjectDependencyGraphStub.Y, ProjectDependencyGraphStub.Z ), currentProjects );
+ }
+
+ static void set( Object obj, String field, Object v ) throws NoSuchFieldException, IllegalAccessException
+ {
+ Field f = obj.getClass().getDeclaredField( field );
+ f.setAccessible( true );
+ f.set( obj, v );
Review Comment:
Alright, thanks!
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1167990027
> > I can write one for [MNG-7474](https://issues.apache.org/jira/browse/MNG-7474). [MNG-7401](https://issues.apache.org/jira/browse/MNG-7401) is an internal behavior and would require a junit test rather than an IT imho.
>
> I guess so that would be best
Unit test and IT added !
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
laeubi commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1131687007
> We should maybe also deprecate this method and suggest that plugins should have the project injected in a field
I think the problem is that we then need some kind of "ProjectScope", I think `MavenSession.set/getCurrentProject()` should simply become a ThreadLocal, we document that and if one needs to pass data to a different thread he first need to fetch the current project.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet merged pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
gnodet merged PR #743:
URL: https://github.com/apache/maven/pull/743
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1166493359
> I can write one for [MNG-7474](https://issues.apache.org/jira/browse/MNG-7474). [MNG-7401](https://issues.apache.org/jira/browse/MNG-7401) is an internal behavior and would require a junit test rather than an IT imho.
I guess so that would be best
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1128898423
> Not sure why tests aren't running. I'll close this one and recreate a new PR.
Ah, closing and reopening did the trick.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1128894857
Not sure why tests aren't running. I'll close this one and recreate a new 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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1165979050
I can write one for MNG-7474.
MNG-7401 is an internal behavior and would require a junit test rather than an IT imho.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1130439258
We have TLS-based solutions in past releases and we had to revert. What makes this one different?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1131622335
> We have TLS-based solutions in past releases and we had to revert. What makes this one different?
It's not a new TLS, it's a move of the existing TLS from SessionScope to MavenSession.
The problem I'm trying to solve is really MNG-7474 : the fact that session scoped beans are not singletons anymore, since the introduction of _multiple_ sessions when introducing the concurrent builds. This is what introduced the TLS in the SessionScope, thereby breaking its purpose, i.e. beans are not cached within the session scope.
So there are several solutions to this problem, but I think the SessionScope is useless in the current form. So if we don't want to use a single session and keep things the way they are, I'd need to introduce a more global scope with a real singleton session object. We can't simply change the SessionScope, because beans are injected with the MavenSession, so if we want singletons, we need to have a consistent MavenSession object across mojo executions, which is not the case because of the multiple session clones.
So the possibilities are:
* revert the use of multiple sessions, i.e. use a single session throughout the whole build, which is what this PR is about. The possible drawback is breakages of code that would use `MavenSession.getCurrentProject()` from a different thread
* introduce a new concept of singleton session + scope. if we want it immutable, it would basically be the MavenSession without the current project. The obvious drawback
* do nothing to fix the problem: this is a possibility and I'd have to work around the problem in m-build-cache-e by keeping state inside the MavenSession, or most probably in the `RepositorySystemSession.getData()` structure. In such a case, I think it would be better to deprecate the `@SessionScope` and related classes.
I think the cleaner state would be to fix the `@SessionScope` and use a singleton for `MavenSession` because that's semantically the most desirable state. Scopes are natural for DI, so it makes sense to use them, and the `MavenSession` cloning stuff has only been introduced to solve the `getCurrentProject()` problem. We should maybe also deprecate this method and suggest that plugins should have the project injected in a field. It would be difficult to get rid of it in the very short term, as that one is used a lot internally for the moment. We could also move it the `LegacySupport` along with the access to the current session.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet closed pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
gnodet closed pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
URL: https://github.com/apache/maven/pull/743
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
laeubi commented on PR #743:
URL: https://github.com/apache/maven/pull/743#issuecomment-1131684537
> revert the use of multiple sessions, i.e. use a single session throughout the whole build, which is what this PR is about. The possible drawback is breakages of code that would use `MavenSession.getCurrentProject()` from a different thread
I think this is (at least for the Tycho scope of the problem) the most noteable thing, and we use `LegacySupport` what effectivly *is* a Threadlocal, so from what I can tell, in almost all cases `MavenSession.getCurrentProject()` returning the current project "of the thread" would be sufficient and cloning the session seem to be effectively for that purpose.
The problem with the cloning is, that if I have a sessionscoped componet I get the "rootsession" and thus calling `MavenSession.getCurrentProject()` most of the time returns `null` as the actual project is set on a cloned copy.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #743: [3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #743:
URL: https://github.com/apache/maven/pull/743#discussion_r928140254
##########
maven-core/src/test/java/org/apache/maven/lifecycle/internal/LifecycleModuleBuilderTest.java:
##########
@@ -0,0 +1,102 @@
+package org.apache.maven.lifecycle.internal;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.maven.execution.AbstractExecutionListener;
+import org.apache.maven.execution.DefaultMavenExecutionRequest;
+import org.apache.maven.execution.DefaultMavenExecutionResult;
+import org.apache.maven.execution.MavenExecutionRequest;
+import org.apache.maven.execution.MavenExecutionResult;
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.lifecycle.LifecycleExecutionException;
+import org.apache.maven.lifecycle.internal.builder.BuilderCommon;
+import org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder;
+import org.apache.maven.lifecycle.internal.stub.DefaultLifecyclesStub;
+import org.apache.maven.lifecycle.internal.stub.LifecycleTaskSegmentCalculatorStub;
+import org.apache.maven.lifecycle.internal.stub.LoggerStub;
+import org.apache.maven.lifecycle.internal.stub.MojoExecutorStub;
+import org.apache.maven.lifecycle.internal.stub.ProjectDependencyGraphStub;
+import org.apache.maven.plugin.MojoExecution;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.ContainerConfiguration;
+import org.codehaus.plexus.PlexusConstants;
+import org.codehaus.plexus.PlexusTestCase;
+import org.codehaus.plexus.logging.Logger;
+import org.junit.Test;
+
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+
+public class LifecycleModuleBuilderTest extends PlexusTestCase
+{
+ @Override
+ protected void customizeContainerConfiguration( ContainerConfiguration configuration )
+ {
+ configuration.setAutoWiring( true );
+ configuration.setClassPathScanning( PlexusConstants.SCANNING_INDEX );
+
+ }
+
+ public void testCurrentProject() throws Exception
+ {
+ List<MavenProject> currentProjects = new ArrayList<>();
+ MojoExecutorStub mojoExecutor = new MojoExecutorStub()
+ {
+ @Override
+ public void execute( MavenSession session, List<MojoExecution> mojoExecutions, ProjectIndex projectIndex )
+ throws LifecycleExecutionException
+ {
+ super.execute( session, mojoExecutions, projectIndex );
+ currentProjects.add( session.getCurrentProject() );
+ }
+ };
+
+ final DefaultMavenExecutionResult defaultMavenExecutionResult = new DefaultMavenExecutionResult();
+ MavenExecutionRequest mavenExecutionRequest = new DefaultMavenExecutionRequest();
+ mavenExecutionRequest.setExecutionListener( new AbstractExecutionListener() );
+ mavenExecutionRequest.setGoals( Arrays.asList( "clean" ) );
+ mavenExecutionRequest.setDegreeOfConcurrency( 1 );
Review Comment:
This is the default value. Any specific reason to set it explicitly?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org