Introduce ResolverLock, layer on top of low level Named Locks

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java

@@ -70,6 +70,7 @@
 import org.eclipse.aether.spi.io.FileProcessor;
 import org.eclipse.aether.spi.locator.Service;
 import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.spi.synccontext.SyncContextHint;
 import org.eclipse.aether.transfer.ArtifactNotFoundException;
 import org.eclipse.aether.transfer.ArtifactTransferException;
 import org.eclipse.aether.transfer.NoRepositoryConnectorException;
@@ -214,21 +215,29 @@ public List<ArtifactResult> resolveArtifacts( RepositorySystemSession session,
         throws ArtifactResolutionException
     {
 
-        try ( SyncContext syncContext = syncContextFactory.newInstance( session, false ) )
+        SyncContextHint.SCOPE.set( SyncContextHint.Scope.RESOLVE ); // HACK
+        try
         {
-            Collection<Artifact> artifacts = new ArrayList<>( requests.size() );
-            for ( ArtifactRequest request : requests )
+            try ( SyncContext syncContext = syncContextFactory.newInstance( session, false ) )
             {
-                if ( request.getArtifact().getProperty( ArtifactProperties.LOCAL_PATH, null ) != null )
+                Collection<Artifact> artifacts = new ArrayList<>( requests.size() );
+                for ( ArtifactRequest request : requests )
                 {
-                    continue;
+                    if ( request.getArtifact().getProperty( ArtifactProperties.LOCAL_PATH, null ) != null )
+                    {
+                        continue;
+                    }
+                    artifacts.add( request.getArtifact() );
                 }
-                artifacts.add( request.getArtifact() );
-            }
 
-            syncContext.acquire( artifacts, null );
+                syncContext.acquire( artifacts, null );
 
-            return resolve( session, requests );
+                return resolve( session, requests );
+            }
+        }
+        finally
+        {
+            SyncContextHint.SCOPE.set( null );
         }
     }
 

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultMetadataResolver.java

@@ -66,6 +66,7 @@
 import org.eclipse.aether.spi.connector.RepositoryConnector;
 import org.eclipse.aether.spi.locator.Service;
 import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.spi.synccontext.SyncContextHint;
 import org.eclipse.aether.transfer.MetadataNotFoundException;
 import org.eclipse.aether.transfer.MetadataTransferException;
 import org.eclipse.aether.transfer.NoRepositoryConnectorException;
@@ -170,17 +171,25 @@ public List<MetadataResult> resolveMetadata( RepositorySystemSession session,
                                                  Collection<? extends MetadataRequest> requests )
     {
 
-        try ( SyncContext syncContext = syncContextFactory.newInstance( session, false ) )
+        SyncContextHint.SCOPE.set( SyncContextHint.Scope.RESOLVE ); // HACK
+        try
         {
-            Collection<Metadata> metadata = new ArrayList<>( requests.size() );
-            for ( MetadataRequest request : requests )
+            try ( SyncContext syncContext = syncContextFactory.newInstance( session, false ) )
             {
-                metadata.add( request.getMetadata() );
-            }
+                Collection<Metadata> metadata = new ArrayList<>( requests.size() );
+                for ( MetadataRequest request : requests )
+                {
+                    metadata.add( request.getMetadata() );
+                }
 
-            syncContext.acquire( null, metadata );
+                syncContext.acquire( null, metadata );
 
-            return resolve( session, requests );
+                return resolve( session, requests );
+            }
+        }
+        finally
+        {
+            SyncContextHint.SCOPE.set( null ); // HACK
         }
     }
 

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/NamedSyncContextFactory.java

@@ -25,6 +25,7 @@
 import org.eclipse.aether.internal.impl.synccontext.named.GAVNameMapper;
 import org.eclipse.aether.internal.impl.synccontext.named.NameMapper;
 import org.eclipse.aether.internal.impl.synccontext.named.NamedLockFactoryAdapter;
+import org.eclipse.aether.internal.impl.synccontext.named.ResolverLockFactory;
 import org.eclipse.aether.internal.impl.synccontext.named.StaticNameMapper;
 import org.eclipse.aether.named.NamedLockFactory;
 import org.eclipse.aether.named.providers.LocalReadWriteLockNamedLockFactory;
@@ -107,7 +108,7 @@ private static NamedLockFactoryAdapter selectAndAdapt( final Map<String, NameMap
             throw new IllegalArgumentException( "Unknown NamedLockFactory name: " + FACTORY_NAME
                     + ", known ones: " + factories.keySet() );
         }
-        return new NamedLockFactoryAdapter( nameMapper, factory, TIME, TIME_UNIT );
+        return new NamedLockFactoryAdapter( new ResolverLockFactory( nameMapper, factory ), TIME, TIME_UNIT );
     }
 
     @Override

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/DiscriminatingNameMapper.java

@@ -34,19 +34,16 @@
 import java.net.InetAddress;
 import java.net.UnknownHostException;
 import java.nio.charset.StandardCharsets;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.Map;
 import java.util.Objects;
 
-import static java.util.stream.Collectors.toList;
-
 /**
  * Discriminating {@link NameMapper}, that wraps another {@link NameMapper} and adds a "discriminator" as prefix, that
  * makes lock names unique including the hostname and local repository (by default). The discriminator may be passed
  * in via {@link RepositorySystemSession} or is automatically calculated based on the local hostname and repository
  * path. The implementation retains order of collection elements as it got it from
- * {@link NameMapper#nameLocks(RepositorySystemSession, Collection, Collection)} method.
+ * {@link NameMapper#nameLock(RepositorySystemSession, Artifact)} method.
  * <p>
  * The default setup wraps {@link GAVNameMapper}, but manually may be created any instance needed.
  */
@@ -84,13 +81,17 @@ public DiscriminatingNameMapper( @Named( GAVNameMapper.NAME ) final NameMapper n
     }
 
     @Override
-    public Collection<String> nameLocks( final RepositorySystemSession session,
-                                         final Collection<? extends Artifact> artifacts,
-                                         final Collection<? extends Metadata> metadatas )
+    public String nameLock( final RepositorySystemSession session, final Artifact artifact )
+    {
+        String discriminator = createDiscriminator( session );
+        return discriminator + ":" + nameMapper.nameLock( session, artifact );
+    }
+
+    @Override
+    public String nameLock( final RepositorySystemSession session, Metadata metadata )
     {
         String discriminator = createDiscriminator( session );
-        return nameMapper.nameLocks( session, artifacts, metadatas ).stream().map( s -> discriminator + ":" + s )
-                         .collect( toList() );
+        return discriminator + ":" + nameMapper.nameLock( session, metadata );
     }
 
     private String getHostname()

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/GAVNameMapper.java

@@ -25,8 +25,6 @@
 
 import javax.inject.Named;
 import javax.inject.Singleton;
-import java.util.Collection;
-import java.util.TreeSet;
 
 /**
  * Artifact GAV {@link NameMapper}, uses artifact and metadata coordinates to name their corresponding locks. Is not
@@ -39,44 +37,29 @@ public class GAVNameMapper implements NameMapper
     public static final String NAME = "gav";
 
     @Override
-    public Collection<String> nameLocks( final RepositorySystemSession session,
-                                         final Collection<? extends Artifact> artifacts,
-                                         final Collection<? extends Metadata> metadatas )
+    public String nameLock( final RepositorySystemSession session, final Artifact artifact )
     {
-        // Deadlock prevention: https://stackoverflow.com/a/16780988/696632
-        // We must acquire multiple locks always in the same order!
-        Collection<String> keys = new TreeSet<>();
-        if ( artifacts != null )
-        {
-            for ( Artifact artifact : artifacts )
-            {
-                String key = "artifact:" + artifact.getGroupId()
-                             + ":" + artifact.getArtifactId()
-                             + ":" + artifact.getBaseVersion();
-                keys.add( key );
-            }
-        }
+        return "artifact:" + artifact.getGroupId()
+               + ":" + artifact.getArtifactId()
+               + ":" + artifact.getBaseVersion();
+    }
 
-        if ( metadatas != null )
+    @Override
+    public String nameLock( final RepositorySystemSession session, final Metadata metadata )
+    {
+        StringBuilder key = new StringBuilder( "metadata:" );
+        if ( !metadata.getGroupId().isEmpty() )
         {
-            for ( Metadata metadata : metadatas )
+            key.append( metadata.getGroupId() );
+            if ( !metadata.getArtifactId().isEmpty() )
             {
-                StringBuilder key = new StringBuilder( "metadata:" );
-                if ( !metadata.getGroupId().isEmpty() )
+                key.append( ':' ).append( metadata.getArtifactId() );
+                if ( !metadata.getVersion().isEmpty() )
                 {
-                    key.append( metadata.getGroupId() );
-                    if ( !metadata.getArtifactId().isEmpty() )
-                    {
-                        key.append( ':' ).append( metadata.getArtifactId() );
-                        if ( !metadata.getVersion().isEmpty() )
-                        {
-                            key.append( ':' ).append( metadata.getVersion() );
-                        }
-                    }
+                    key.append( ':' ).append( metadata.getVersion() );
                 }
-                keys.add( key.toString() );
             }
         }
-        return keys;
+        return key.toString();
     }
 }

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NameMapper.java

@@ -23,8 +23,6 @@
 import org.eclipse.aether.artifact.Artifact;
 import org.eclipse.aether.metadata.Metadata;
 
-import java.util.Collection;
-
 /**
  * Component mapping lock names to passed in artifacts and metadata as required.
  */
@@ -37,6 +35,7 @@ public interface NameMapper
      * same criteria) to avoid deadlocks by acquiring locks in same order, essentially disregarding the order of
      * the input collections.
      */
-    Collection<String> nameLocks( RepositorySystemSession session, Collection<? extends Artifact> artifacts,
-                                  Collection<? extends Metadata> metadatas );
+    String nameLock( RepositorySystemSession session, Artifact artifact );
+
+    String nameLock( RepositorySystemSession session, Metadata metadata );
 }

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/named/NamedLockFactoryAdapter.java

@@ -39,19 +39,16 @@
  */
 public final class NamedLockFactoryAdapter
 {
-    private final NameMapper nameMapper;
-
-    private final NamedLockFactory namedLockFactory;
+    private final ResolverLockFactory resolverLockFactory;
 
     private final long time;
 
     private final TimeUnit timeUnit;
 
-    public NamedLockFactoryAdapter( final NameMapper nameMapper, final NamedLockFactory namedLockFactory,
+    public NamedLockFactoryAdapter( final ResolverLockFactory resolverLockFactory,
                                     final long time, final TimeUnit timeUnit )
     {
-        this.nameMapper = Objects.requireNonNull( nameMapper );
-        this.namedLockFactory = Objects.requireNonNull( namedLockFactory );
+        this.resolverLockFactory = resolverLockFactory;
         if ( time < 0L )
         {
             throw new IllegalArgumentException( "time cannot be negative" );
@@ -62,12 +59,12 @@ public NamedLockFactoryAdapter( final NameMapper nameMapper, final NamedLockFact
 
     public SyncContext newInstance( final RepositorySystemSession session, final boolean shared )
     {
-        return new AdaptedLockSyncContext( session, shared, nameMapper, namedLockFactory, time, timeUnit );
+        return new AdaptedLockSyncContext( session, shared, resolverLockFactory, time, timeUnit );
     }
 
     public void shutdown()
     {
-        namedLockFactory.shutdown();
+        resolverLockFactory.shutdown();
     }
 
     private static class AdaptedLockSyncContext implements SyncContext
@@ -78,28 +75,21 @@ private static class AdaptedLockSyncContext implements SyncContext
 
         private final boolean shared;
 
-        private final NameMapper lockNaming;
-
-        private final SessionAwareNamedLockFactory sessionAwareNamedLockFactory;
-
-        private final NamedLockFactory namedLockFactory;
+        private final ResolverLockFactory resolverLockFactory;
 
         private final long time;
 
         private final TimeUnit timeUnit;
 
-        private final Deque<NamedLock> locks;
+        private final Deque<ResolverLock> locks;
 
         private AdaptedLockSyncContext( final RepositorySystemSession session, final boolean shared,
-                                        final NameMapper lockNaming, final NamedLockFactory namedLockFactory,
+                                        final ResolverLockFactory resolverLockFactory,
                                         final long time, final TimeUnit timeUnit )
         {
             this.session = session;
             this.shared = shared;
-            this.lockNaming = lockNaming;
-            this.sessionAwareNamedLockFactory = namedLockFactory instanceof SessionAwareNamedLockFactory
-                    ? (SessionAwareNamedLockFactory) namedLockFactory : null;
-            this.namedLockFactory = namedLockFactory;
+            this.resolverLockFactory = resolverLockFactory;
             this.time = time;
             this.timeUnit = timeUnit;
             this.locks = new ArrayDeque<>();
@@ -108,45 +98,35 @@ private AdaptedLockSyncContext( final RepositorySystemSession session, final boo
         @Override
         public void acquire( Collection<? extends Artifact> artifacts, Collection<? extends Metadata> metadatas )
         {
-            Collection<String> keys = lockNaming.nameLocks( session, artifacts, metadatas );
-            if ( keys.isEmpty() )
+            Collection<ResolverLock> resolverLocks = resolverLockFactory.resolverLocks(
+                    session, shared, artifacts, metadatas );
+            if ( resolverLocks.isEmpty() )
             {
                 return;
             }
 
-            LOGGER.trace( "Need {} {} lock(s) for {}", keys.size(), shared ? "read" : "write", keys );
+            LOGGER.trace( "Need {} {} lock(s) for {}", resolverLocks.size(), shared ? "read" : "write", resolverLocks );
             int acquiredLockCount = 0;
-            for ( String key : keys )
+            for ( ResolverLock resolverLock : resolverLocks )
             {
-                NamedLock namedLock = sessionAwareNamedLockFactory != null ? sessionAwareNamedLockFactory
-                        .getLock( session, key ) : namedLockFactory.getLock( key );
                 try
                 {
-                     LOGGER.trace( "Acquiring {} lock for '{}'",
-                             shared ? "read" : "write", key );
-
-                    boolean locked;
-                    if ( shared )
-                    {
-                        locked = namedLock.lockShared( time, timeUnit );
-                    }
-                    else
-                    {
-                        locked = namedLock.lockExclusively( time, timeUnit );
-                    }
+                     LOGGER.trace( "Acquiring effective {} lock for '{}'",
+                             resolverLock.isEffectiveShared() ? "read" : "write", resolverLock.key() );
 
+                    boolean locked = resolverLock.tryLock( time, timeUnit );
                     if ( !locked )
                     {
-                        LOGGER.trace( "Failed to acquire {} lock for '{}'",
-                                shared ? "read" : "write", key );
+                        LOGGER.trace( "Failed to acquire effective {} lock for '{}'",
+                                resolverLock.isEffectiveShared() ? "read" : "write", resolverLock.key() );
 
-                        namedLock.close();
+                        resolverLock.close();
                         throw new IllegalStateException(
-                                "Could not acquire " + ( shared ? "read" : "write" )
-                                + " lock for '" + namedLock.name() + "'" );
+                                "Could not acquire effective " + ( resolverLock.isEffectiveShared() ? "read" : "write" )
+                                + " lock for '" + resolverLock.key() + "'" );
                     }
 
-                    locks.push( namedLock );
+                    locks.push( resolverLock );
                     acquiredLockCount++;
                 }
                 catch ( InterruptedException e )
@@ -170,11 +150,11 @@ public void close()
             int released = 0;
             while ( !locks.isEmpty() )
             {
-                try ( NamedLock namedLock = locks.pop() )
+                try ( ResolverLock resolverLock = locks.pop() )
                 {
                     LOGGER.trace( "Releasing {} lock for '{}'",
-                            shared ? "read" : "write", namedLock.name() );
-                    namedLock.unlock();
+                            shared ? "read" : "write", resolverLock.key() );
+                    resolverLock.unlock();
                     released++;
                 }
             }