[MNG-7347] SessionScoped beans should be singletons for a given sessi…

…on (#621)

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

@@ -89,8 +89,12 @@ public void buildProject( MavenSession session, MavenSession rootSession, Reacto
 
         // 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 );
+        final boolean scoped = session != rootSession;
+        if ( scoped )
+        {
+            sessionScope.enter();
+            sessionScope.seed( MavenSession.class, session );
+        }
         try
         {
 
@@ -144,7 +148,10 @@ public void buildProject( MavenSession session, MavenSession rootSession, Reacto
         }
         finally
         {
-            sessionScope.exit();
+            if ( scoped )
+            {
+                sessionScope.exit();
+            }
 
             session.setCurrentProject( null );
 

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

@@ -122,8 +122,7 @@ public void execute( MavenSession session )
             ClassLoader oldContextClassLoader = Thread.currentThread().getContextClassLoader();
             ReactorBuildStatus reactorBuildStatus = new ReactorBuildStatus( session.getProjectDependencyGraph() );
             reactorContext =
-                new ReactorContext( result, projectIndex, oldContextClassLoader, reactorBuildStatus,
-                                    sessionScope.memento() );
+                new ReactorContext( result, projectIndex, oldContextClassLoader, reactorBuildStatus );
 
             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 @@
  */
 
 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,
-                           SessionScope.Memento sessionScope )
+                           ClassLoader originalContextClassLoader, ReactorBuildStatus reactorBuildStatus )
     {
         this.result = result;
         this.projectIndex = projectIndex;
         this.originalContextClassLoader = originalContextClassLoader;
         this.reactorBuildStatus = reactorBuildStatus;
-        this.sessionScope = sessionScope;
     }
 
     public ReactorBuildStatus getReactorBuildStatus()
@@ -73,11 +68,4 @@ public ClassLoader getOriginalContextClassLoader()
         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,35 +19,23 @@
  * under the License.
  */
 
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.LinkedList;
+import java.util.Collection;
+import java.util.List;
 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
  */
 public class SessionScope
-    implements Scope
+        implements Scope
 {
-    /**
-     * @since 3.3.0
-     */
-    public static class Memento
-    {
-        final Map<Key<?>, Provider<?>> seeded;
-
-        Memento( final Map<Key<?>, Provider<?>> seeded )
-        {
-            this.seeded = Collections.unmodifiableMap( new HashMap<>( seeded ) );
-        }
-    }
 
     private static final Provider<Object> SEEDED_KEY_PROVIDER = () ->
     {
@@ -57,106 +45,99 @@ public static class Memento
     /**
      * 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 void enter()
-    {
-        LinkedList<ScopeState> stack = values.get();
-        if ( stack == null )
+        public Collection<CachingProvider<?>> providers()
         {
-            stack = new LinkedList<>();
-            values.set( stack );
+            return provided.values();
         }
-        stack.addFirst( new ScopeState() );
     }
 
-    /**
-     * @since 3.3.0
-     */
-    public void enter( Memento memento )
+    private final List<ScopeState> values = new CopyOnWriteArrayList<>();
+
+    public void enter()
     {
-        enter();
-        getScopeState().seeded.putAll( memento.seeded );
+        values.add( 0, new ScopeState() );
     }
 
-    private ScopeState getScopeState()
+    protected ScopeState getScopeState()
     {
-        LinkedList<ScopeState> stack = values.get();
-        if ( stack == null || stack.isEmpty() )
+        if ( values.isEmpty() )
         {
-            throw new IllegalStateException();
+            throw new OutOfScopeException( "Cannot access session scope outside of a scoping block" );
         }
-        return stack.getFirst();
+        return values.get( 0 );
     }
 
     public void exit()
     {
-        final LinkedList<ScopeState> stack = values.get();
-        if ( stack == null || stack.isEmpty() )
+        if ( values.isEmpty() )
         {
             throw new IllegalStateException();
         }
-        stack.removeFirst();
-        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.emptyMap() );
+        values.remove( 0 );
     }
 
     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 () ->
-        {
-            LinkedList<ScopeState> stack = values.get();
-            if ( stack == null || stack.isEmpty() )
-            {
-                throw new OutOfScopeException( "Cannot access " + key + " outside of a scoping block" );
-            }
+        // Lazy evaluating provider
+        return () -> getScopeState().scope( key, unscoped ).get();
+    }
 
-            ScopeState state = stack.getFirst();
+    protected static class CachingProvider<T> implements Provider<T>
+    {
+        private final Provider<T> provider;
+        private volatile T value;
 
-            Provider<?> seeded = state.seeded.get( key );
+        CachingProvider( Provider<T> provider )
+        {
+            this.provider = provider;
+        }
 
-            if ( seeded != null )
-            {
-                return (T) seeded.get();
-            }
+        public T value()
+        {
+            return value;
+        }
 
-            T provided = (T) state.provided.get( key );
-            if ( provided == null && unscoped != null )
+        @Override
+        public T get()
+        {
+            if ( value == null )
             {
-                provided = unscoped.get();
-                state.provided.put( key, provided );
+                synchronized ( this )
+                {
+                    if ( value == null )
+                    {
+                        value = provider.get();
+                    }
+                }
             }
-
-            return provided;
-        };
+            return value;
+        }
     }
 
     @SuppressWarnings( { "unchecked" } )

maven-core/src/test/java/org/apache/maven/session/scope/internal/SessionScopeTest.java

@@ -0,0 +1,81 @@
+package org.apache.maven.session.scope.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 javax.inject.Provider;
+
+import com.google.inject.Key;
+import com.google.inject.OutOfScopeException;
+import org.apache.maven.model.building.DefaultModelSourceTransformer;
+import org.apache.maven.model.building.ModelSourceTransformer;
+import org.apache.maven.model.locator.DefaultModelLocator;
+import org.apache.maven.model.locator.ModelLocator;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+public class SessionScopeTest {
+
+    @Test
+    public void testScope() throws Exception
+    {
+        SessionScope scope = new SessionScope();
+
+        assertThrows( OutOfScopeException.class, () -> scope.seed( ModelLocator.class, new DefaultModelLocator() ) );
+
+        Provider<ModelLocator> pml = scope.scope( Key.get( ModelLocator.class), DefaultModelLocator::new );
+        assertNotNull( pml );
+        assertThrows( OutOfScopeException.class, pml::get );
+
+        Provider<ModelSourceTransformer> pmst = scope.scope( Key.get( ModelSourceTransformer.class ), DefaultModelSourceTransformer::new );
+        assertNotNull( pmst );
+
+        scope.enter();
+
+        final DefaultModelLocator dml1 = new DefaultModelLocator();
+        scope.seed( ModelLocator.class, dml1 );
+
+        assertSame( dml1, pml.get() );
+
+        ModelSourceTransformer mst1 = pmst.get();
+        assertSame( mst1, pmst.get() );
+        Provider<ModelSourceTransformer> pmst1 = scope.scope( Key.get( ModelSourceTransformer.class ), DefaultModelSourceTransformer::new );
+        assertNotNull( pmst1 );
+        assertSame( mst1, pmst1.get() );
+
+        scope.enter();
+
+        pmst1 = scope.scope( Key.get( ModelSourceTransformer.class ), DefaultModelSourceTransformer::new );
+        assertNotNull( pmst1 );
+        assertNotSame( mst1, pmst1.get() );
+
+        scope.exit();
+
+        assertSame( mst1, pmst.get() );
+
+        scope.exit();
+
+        assertThrows( OutOfScopeException.class, pmst::get );
+        assertThrows( OutOfScopeException.class, () -> scope.seed( ModelLocator.class, new DefaultModelLocator() ) );
+    }
+}