[3.9.x] [MNG-7401] [MNG-7474] Keep a single maven session and fix session scope (#743)

* [MNG-7474] SessionScoped beans should be singletons for a given session

Now that the Session is not cloned anymore, we can revert to the original
(Maven < 3.3) behavior of the session scoped components.

Co-authored-by: Christoph Läubrich <christoph@laeubi-soft.de>

This closes #743

* Remove setting a value which is the default already

Co-authored-by: Christoph Läubrich <christoph@laeubi-soft.de>

maven-core/src/main/java/org/apache/maven/execution/MavenSession.java

@@ -54,7 +54,7 @@ public class MavenSession
 
     private Properties executionProperties;
 
-    private MavenProject currentProject;
+    private ThreadLocal<MavenProject> currentProject = new ThreadLocal<>();
 
     /**
      * These projects have already been topologically sorted in the {@link org.apache.maven.Maven} component before
@@ -83,14 +83,15 @@ public class MavenSession
     {
         if ( !projects.isEmpty() )
         {
-            this.currentProject = projects.get( 0 );
+            MavenProject first = projects.get( 0 );
+            this.currentProject = ThreadLocal.withInitial( () -> first );
             this.topLevelProject =
                     projects.stream().filter( project -> project.isExecutionRoot() ).findFirst()
-                            .orElse( currentProject );
+                            .orElse( first );
         }
         else
         {
-            this.currentProject = null;
+            this.currentProject = new ThreadLocal<>();
             this.topLevelProject = null;
         }
         this.projects = projects;
@@ -151,12 +152,12 @@ public class MavenSession
 
     public void setCurrentProject( MavenProject currentProject )
     {
-        this.currentProject = currentProject;
+        this.currentProject.set( currentProject );
     }
 
     public MavenProject getCurrentProject()
     {
-        return currentProject;
+        return currentProject.get();
     }
 
     public ProjectBuildingRequest getProjectBuildingRequest()
@@ -233,7 +234,12 @@ public class MavenSession
     {
         try
         {
-            return (MavenSession) super.clone();
+            MavenSession clone = (MavenSession) super.clone();
+            // the default must become the current project of the thread that clones this
+            MavenProject current = getCurrentProject();
+            // we replace the thread local of the clone to prevent write through and enforce the new default value
+            clone.currentProject = ThreadLocal.withInitial( () -> current );
+            return clone;
         }
         catch ( CloneNotSupportedException e )
         {

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

@@ -58,15 +58,16 @@ public class BuildListCalculator
             for ( MavenProject project : projects )
             {
                 ClassLoader tccl = Thread.currentThread().getContextClassLoader();
+                MavenProject currentProject = session.getCurrentProject();
                 try
                 {
                     BuilderCommon.attachToThread( project ); // Not totally sure if this is needed for anything
-                    MavenSession copiedSession = session.clone();
+                    session.setCurrentProject( project );
-                    copiedSession.setCurrentProject( project );
+                    projectBuilds.add( new ProjectSegment( project, taskSegment, session ) );
-                    projectBuilds.add( new ProjectSegment( project, taskSegment, copiedSession ) );
                 }
                 finally
                 {
+                    session.setCurrentProject( currentProject );
                     Thread.currentThread().setContextClassLoader( tccl );
                 }
             }

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

@@ -32,7 +32,6 @@ import org.apache.maven.lifecycle.MavenExecutionPlan;
 import org.apache.maven.lifecycle.internal.builder.BuilderCommon;
 import org.apache.maven.plugin.MojoExecution;
 import org.apache.maven.project.MavenProject;
-import org.apache.maven.session.scope.internal.SessionScope;
 import org.codehaus.plexus.component.annotations.Component;
 import org.codehaus.plexus.component.annotations.Requirement;
 
@@ -66,9 +65,6 @@ public class LifecycleModuleBuilder
     @Requirement
     private List<ProjectExecutionListener> projectExecutionListeners;
 
-    @Requirement
-    private SessionScope sessionScope;
-
     public void setProjectExecutionListeners( final List<ProjectExecutionListener> listeners )
     {
         this.projectExecutionListeners = listeners;
@@ -88,10 +84,6 @@ public class LifecycleModuleBuilder
 
         long buildStartTime = System.currentTimeMillis();
 
-        // session may be different from rootSession seeded in DefaultMaven
-        // explicitly seed the right session here to make sure it is used by Guice
-        sessionScope.enter( reactorContext.getSessionScopeMemento() );
-        sessionScope.seed( MavenSession.class, session );
         try
         {
 
@@ -145,8 +137,6 @@ public class LifecycleModuleBuilder
         }
         finally
         {
-            sessionScope.exit();
-
             session.setCurrentProject( null );
 
             Thread.currentThread().setContextClassLoader( reactorContext.getOriginalContextClassLoader() );

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

@@ -106,9 +106,7 @@ public class LifecycleStarter
 
             ClassLoader oldContextClassLoader = Thread.currentThread().getContextClassLoader();
             ReactorBuildStatus reactorBuildStatus = new ReactorBuildStatus( session.getProjectDependencyGraph() );
-            reactorContext =
+            reactorContext = new ReactorContext( result, projectIndex, oldContextClassLoader, reactorBuildStatus );
-                new ReactorContext( result, projectIndex, oldContextClassLoader, reactorBuildStatus,
-                                    sessionScope.memento() );
 
             String builderId = session.getRequest().getBuilderId();
             Builder builder = builders.get( builderId );

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

@@ -20,7 +20,6 @@ package org.apache.maven.lifecycle.internal;
  */
 
 import org.apache.maven.execution.MavenExecutionResult;
-import org.apache.maven.session.scope.internal.SessionScope;
 
 /**
  * Context that is fixed for the entire reactor build.
@@ -40,17 +39,13 @@ public class ReactorContext
 
     private final ReactorBuildStatus reactorBuildStatus;
 
-    private final SessionScope.Memento sessionScope;
-
     public ReactorContext( MavenExecutionResult result, ProjectIndex projectIndex,
-                           ClassLoader originalContextClassLoader, ReactorBuildStatus reactorBuildStatus,
+                           ClassLoader originalContextClassLoader, ReactorBuildStatus reactorBuildStatus )
-                           SessionScope.Memento sessionScope )
     {
         this.result = result;
         this.projectIndex = projectIndex;
         this.originalContextClassLoader = originalContextClassLoader;
         this.reactorBuildStatus = reactorBuildStatus;
-        this.sessionScope = sessionScope;
     }
 
     public ReactorBuildStatus getReactorBuildStatus()
@@ -73,11 +68,4 @@ public class ReactorContext
         return originalContextClassLoader;
     }
 
-    /**
-     * @since 3.3.0
-     */
-    public SessionScope.Memento getSessionScopeMemento()
-    {
-        return sessionScope;
-    }
 }

maven-core/src/main/java/org/apache/maven/session/scope/internal/SessionScope.java

@@ -19,16 +19,16 @@ package org.apache.maven.session.scope.internal;
  * under the License.
  */
 
-import java.util.Collections;
+import java.util.Collection;
-import java.util.HashMap;
+import java.util.List;
-import java.util.LinkedList;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import com.google.inject.Key;
 import com.google.inject.OutOfScopeException;
 import com.google.inject.Provider;
 import com.google.inject.Scope;
-import com.google.inject.util.Providers;
 
 /**
  * SessionScope
@@ -36,134 +36,112 @@ import com.google.inject.util.Providers;
 public class SessionScope
     implements Scope
 {
-    /**
-     * @since 3.3.0
-     */
-    public static class Memento
-    {
-        final Map<Key<?>, Provider<?>> seeded;
 
-        Memento( final Map<Key<?>, Provider<?>> seeded )
+    private static final Provider<Object> SEEDED_KEY_PROVIDER = () ->
-        {
-            this.seeded = Collections.unmodifiableMap( new HashMap<>( seeded ) );
-        }
-    }
-
-    private static final Provider<Object> SEEDED_KEY_PROVIDER = new Provider<Object>()
-    {
-        public Object get()
     {
         throw new IllegalStateException();
-        }
     };
 
     /**
      * ScopeState
      */
-    private static final class ScopeState
+    protected static final class ScopeState
     {
-        private final Map<Key<?>, Provider<?>> seeded = new HashMap<>();
+        private final Map<Key<?>, CachingProvider<?>> provided = new ConcurrentHashMap<>();
 
-        private final Map<Key<?>, Object> provided = new HashMap<>();
+        public <T> void seed( Class<T> clazz, Provider<T> value )
+        {
+            provided.put( Key.get( clazz ), new CachingProvider<>( value ) );
         }
 
-    private final ThreadLocal<LinkedList<ScopeState>> values = new ThreadLocal<>();
+        @SuppressWarnings( "unchecked" )
+        public <T> Provider<T> scope( Key<T> key, Provider<T> unscoped )
+        {
+            Provider<?> provider = provided.computeIfAbsent( key, k -> new CachingProvider<>( unscoped ) );
+            return ( Provider<T> ) provider;
+        }
+
+        public Collection<CachingProvider<?>> providers()
+        {
+            return provided.values();
+        }
+    }
+
+    private final List<ScopeState> values = new CopyOnWriteArrayList<>();
 
     public void enter()
     {
-        LinkedList<ScopeState> stack = values.get();
+        values.add( 0, new ScopeState() );
-        if ( stack == null )
-        {
-            stack = new LinkedList<>();
-            values.set( stack );
-        }
-        stack.addFirst( new ScopeState() );
     }
 
-    /**
+    protected ScopeState getScopeState()
-     * @since 3.3.0
-     */
-    public void enter( Memento memento )
     {
-        enter();
+        if ( values.isEmpty() )
-        getScopeState().seeded.putAll( memento.seeded );
+        {
+            throw new OutOfScopeException( "Cannot access session scope outside of a scoping block" );
         }
-
+        return values.get( 0 );
-    private ScopeState getScopeState()
-    {
-        LinkedList<ScopeState> stack = values.get();
-        if ( stack == null || stack.isEmpty() )
-        {
-            throw new IllegalStateException();
-        }
-        return stack.getFirst();
     }
 
     public void exit()
     {
-        final LinkedList<ScopeState> stack = values.get();
+        if ( values.isEmpty() )
-        if ( stack == null || stack.isEmpty() )
         {
             throw new IllegalStateException();
         }
-        stack.removeFirst();
+        values.remove( 0 );
-        if ( stack.isEmpty() )
-        {
-            values.remove();
-        }
-    }
-
-    /**
-     * @since 3.3.0
-     */
-    public Memento memento()
-    {
-        LinkedList<ScopeState> stack = values.get();
-        return new Memento( stack != null ? stack.getFirst().seeded : Collections.<Key<?>, Provider<?>>emptyMap() );
     }
 
     public <T> void seed( Class<T> clazz, Provider<T> value )
     {
-        getScopeState().seeded.put( Key.get( clazz ), value );
+        getScopeState().seed( clazz, value );
     }
 
     public <T> void seed( Class<T> clazz, final T value )
     {
-        getScopeState().seeded.put( Key.get( clazz ), Providers.of( value ) );
+        seed( clazz, ( Provider<T> ) () -> value );
     }
 
     public <T> Provider<T> scope( final Key<T> key, final Provider<T> unscoped )
     {
-        return new Provider<T>()
+        // Lazy evaluating provider
+        return () -> getScopeState().scope( key, unscoped ).get();
+    }
+
+    /**
+     * A provider wrapping an existing provider with a cache
+     * @param <T> the provided type
+     */
+    protected static class CachingProvider<T> implements Provider<T>
     {
-            @SuppressWarnings( "unchecked" )
+        private final Provider<T> provider;
+        private volatile T value;
+
+        CachingProvider( Provider<T> provider )
+        {
+            this.provider = provider;
+        }
+
+        public T value()
+        {
+            return value;
+        }
+
+        @Override
         public T get()
         {
-                LinkedList<ScopeState> stack = values.get();
+            if ( value == null )
-                if ( stack == null || stack.isEmpty() )
             {
-                    throw new OutOfScopeException( "Cannot access " + key + " outside of a scoping block" );
+                synchronized ( this )
-                }
-
-                ScopeState state = stack.getFirst();
-
-                Provider<?> seeded = state.seeded.get( key );
-
-                if ( seeded != null )
                 {
-                    return (T) seeded.get();
+                    if ( value == null )
-                }
-
-                T provided = (T) state.provided.get( key );
-                if ( provided == null && unscoped != null )
                     {
-                    provided = unscoped.get();
+                        value = provider.get();
-                    state.provided.put( key, provided );
                     }
-
-                return provided;
                 }
-        };
+            }
+            return value;
+        }
     }
 
     @SuppressWarnings( { "unchecked" } )

maven-core/src/test/java/org/apache/maven/lifecycle/internal/BuildListCalculatorTest.java

@@ -39,6 +39,11 @@ public class BuildListCalculatorTest
         assertEquals( "Stub data contains 6 items", 6, segments.size() );
         final ProjectSegment build = segments.get( 0 );
         assertNotNull( build );
+
+        for ( ProjectSegment segment : segments )
+        {
+            assertSame( session, segment.getSession() );
+        }
     }
 
     private static LifecycleTaskSegmentCalculator getTaskSegmentCalculator()

maven-core/src/test/java/org/apache/maven/lifecycle/internal/LifecycleModuleBuilderTest.java

@@ -0,0 +1,101 @@
+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" ) );
+        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 );
+    }
+
+}