[MRESOLVER-421] Extend NamedLock API to make possible 1..n lock to name (apache#431)

This PR implements MRESOLVER-421, basically allowing not only 1:1 resource to lock mapping as Named Locks were originally incepted. Also, change lock name to dedicated "key" type, and this improves tremendously logging and diagnostic (ie. if you used hashing name mapper, good luck to tell what artifact it was).

This PR also adds tests to Redisson as there were no tests in the module since its inception.

---

https://issues.apache.org/jira/browse/MRESOLVER-421

Author: cstamas

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

@@ -27,6 +27,7 @@
 import org.eclipse.aether.RepositorySystemSession;
 import org.eclipse.aether.artifact.Artifact;
 import org.eclipse.aether.metadata.Metadata;
+import org.eclipse.aether.named.NamedLockKey;
 import org.eclipse.aether.util.DirectoryUtils;
 
 import static java.util.Objects.requireNonNull;
@@ -82,7 +83,7 @@ public boolean isFileSystemFriendly() {
     }
 
     @Override
-    public Collection<String> nameLocks(
+    public Collection<NamedLockKey> nameLocks(
             final RepositorySystemSession session,
             final Collection<? extends Artifact> artifacts,
             final Collection<? extends Metadata> metadatas) {
@@ -92,7 +93,8 @@ public Collection<String> nameLocks(
                     : DirectoryUtils.resolveDirectory(session, DEFAULT_LOCKS_DIR, CONFIG_PROP_LOCKS_DIR, false);
 
             return delegate.nameLocks(session, artifacts, metadatas).stream()
-                    .map(name -> basedir.resolve(name).toAbsolutePath().toUri().toASCIIString())
+                    .map(k -> NamedLockKey.of(
+                            basedir.resolve(k.name()).toAbsolutePath().toUri().toASCIIString(), k.resources()))
                     .collect(Collectors.toList());
         } catch (IOException e) {
             throw new UncheckedIOException(e);

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

@@ -26,6 +26,7 @@
 import org.eclipse.aether.RepositorySystemSession;
 import org.eclipse.aether.artifact.Artifact;
 import org.eclipse.aether.metadata.Metadata;
+import org.eclipse.aether.named.NamedLockKey;
 import org.eclipse.aether.util.ConfigUtils;
 import org.eclipse.aether.util.StringDigestUtil;
 import org.slf4j.Logger;
@@ -86,13 +87,13 @@ public boolean isFileSystemFriendly() {
     }
 
     @Override
-    public Collection<String> nameLocks(
+    public Collection<NamedLockKey> nameLocks(
             final RepositorySystemSession session,
             final Collection<? extends Artifact> artifacts,
             final Collection<? extends Metadata> metadatas) {
         String discriminator = createDiscriminator(session);
         return delegate.nameLocks(session, artifacts, metadatas).stream()
-                .map(s -> discriminator + ":" + s)
+                .map(k -> NamedLockKey.of(discriminator + ":" + k.name(), k.resources()))
                 .collect(toList());
     }
 

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

@@ -19,11 +19,13 @@
 package org.eclipse.aether.internal.impl.synccontext.named;
 
 import java.util.Collection;
+import java.util.Comparator;
 import java.util.TreeSet;
 
 import org.eclipse.aether.RepositorySystemSession;
 import org.eclipse.aether.artifact.Artifact;
 import org.eclipse.aether.metadata.Metadata;
+import org.eclipse.aether.named.NamedLockKey;
 
 import static java.util.Objects.requireNonNull;
 
@@ -66,49 +68,53 @@ public boolean isFileSystemFriendly() {
     }
 
     @Override
-    public Collection<String> nameLocks(
+    public Collection<NamedLockKey> nameLocks(
             final RepositorySystemSession session,
             final Collection<? extends Artifact> artifacts,
             final Collection<? extends Metadata> metadatas) {
         // Deadlock prevention: https://stackoverflow.com/a/16780988/696632
         // We must acquire multiple locks always in the same order!
-        TreeSet<String> keys = new TreeSet<>();
+        TreeSet<NamedLockKey> keys = new TreeSet<>(Comparator.comparing(NamedLockKey::name));
         if (artifacts != null) {
             for (Artifact artifact : artifacts) {
-                keys.add(getArtifactName(artifact));
+                keys.add(NamedLockKey.of(
+                        getArtifactName(artifact, artifactPrefix, fieldSeparator, artifactSuffix),
+                        getArtifactName(artifact, "", ":", "")));
             }
         }
 
         if (metadatas != null) {
             for (Metadata metadata : metadatas) {
-                keys.add(getMetadataName(metadata));
+                keys.add(NamedLockKey.of(
+                        getMetadataName(metadata, metadataPrefix, fieldSeparator, metadataSuffix),
+                        getMetadataName(metadata, "", ":", "")));
             }
         }
         return keys;
     }
 
-    private String getArtifactName(Artifact artifact) {
-        return artifactPrefix
+    private static String getArtifactName(Artifact artifact, String prefix, String separator, String suffix) {
+        return prefix
                 + artifact.getGroupId()
-                + fieldSeparator
+                + separator
                 + artifact.getArtifactId()
-                + fieldSeparator
+                + separator
                 + artifact.getBaseVersion()
-                + artifactSuffix;
+                + suffix;
     }
 
-    private String getMetadataName(Metadata metadata) {
-        String name = metadataPrefix;
+    private static String getMetadataName(Metadata metadata, String prefix, String separator, String suffix) {
+        String name = prefix;
         if (!metadata.getGroupId().isEmpty()) {
             name += metadata.getGroupId();
             if (!metadata.getArtifactId().isEmpty()) {
-                name += fieldSeparator + metadata.getArtifactId();
+                name += separator + metadata.getArtifactId();
                 if (!metadata.getVersion().isEmpty()) {
-                    name += fieldSeparator + metadata.getVersion();
+                    name += separator + metadata.getVersion();
                 }
             }
         }
-        return name + metadataSuffix;
+        return name + suffix;
     }
 
     public static NameMapper gav() {

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

@@ -24,6 +24,7 @@
 import org.eclipse.aether.RepositorySystemSession;
 import org.eclipse.aether.artifact.Artifact;
 import org.eclipse.aether.metadata.Metadata;
+import org.eclipse.aether.named.NamedLockKey;
 import org.eclipse.aether.util.ConfigUtils;
 import org.eclipse.aether.util.StringDigestUtil;
 
@@ -63,7 +64,7 @@ public boolean isFileSystemFriendly() {
     }
 
     @Override
-    public Collection<String> nameLocks(
+    public Collection<NamedLockKey> nameLocks(
             RepositorySystemSession session,
             Collection<? extends Artifact> artifacts,
             Collection<? extends Metadata> metadatas) {
@@ -72,7 +73,7 @@ public Collection<String> nameLocks(
             throw new IllegalArgumentException("allowed depth value is between 0 and 4 (inclusive)");
         }
         return delegate.nameLocks(session, artifacts, metadatas).stream()
-                .map(n -> hashName(n, depth))
+                .map(k -> NamedLockKey.of(hashName(k.name(), depth), k.resources()))
                 .collect(Collectors.toList());
     }
 

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

@@ -23,6 +23,7 @@
 import org.eclipse.aether.RepositorySystemSession;
 import org.eclipse.aether.artifact.Artifact;
 import org.eclipse.aether.metadata.Metadata;
+import org.eclipse.aether.named.NamedLockKey;
 
 /**
  * Component mapping lock names to passed in artifacts and metadata as required.
@@ -56,7 +57,7 @@ public interface NameMapper {
      * Note: name mapper must not use same string for artifacts and metadata, so even the simplest possible
      * implementation like {@link StaticNameMapper} uses two different static strings.
      */
-    Collection<String> nameLocks(
+    Collection<NamedLockKey> nameLocks(
             RepositorySystemSession session,
             Collection<? extends Artifact> artifacts,
             Collection<? extends Metadata> metadatas);

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

@@ -19,7 +19,6 @@
 package org.eclipse.aether.internal.impl.synccontext.named;
 
 import java.util.ArrayDeque;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Deque;
 import java.util.concurrent.TimeUnit;
@@ -31,6 +30,7 @@
 import org.eclipse.aether.metadata.Metadata;
 import org.eclipse.aether.named.NamedLock;
 import org.eclipse.aether.named.NamedLockFactory;
+import org.eclipse.aether.named.NamedLockKey;
 import org.eclipse.aether.named.providers.FileLockNamedLockFactory;
 import org.eclipse.aether.util.ConfigUtils;
 import org.slf4j.Logger;
@@ -196,87 +196,82 @@ private long getRetryWait(final RepositorySystemSession session) {
 
         @Override
         public void acquire(Collection<? extends Artifact> artifacts, Collection<? extends Metadata> metadatas) {
-            Collection<String> keys = lockNaming.nameLocks(session, artifacts, metadatas);
+            Collection<NamedLockKey> keys = lockNaming.nameLocks(session, artifacts, metadatas);
             if (keys.isEmpty()) {
                 return;
             }
 
+            final String timeStr = time + " " + timeUnit;
+            final String lockKind = shared ? "shared" : "exclusive";
+            final NamedLock namedLock = namedLockFactory.getLock(keys);
+            if (LOGGER.isTraceEnabled()) {
+                LOGGER.trace(
+                        "Need {} lock for {} from {}",
+                        lockKind,
+                        namedLock.key().resources(),
+                        namedLock.key().name());
+            }
+
             final int attempts = retry + 1;
-            final ArrayList<IllegalStateException> illegalStateExceptions = new ArrayList<>();
             for (int attempt = 1; attempt <= attempts; attempt++) {
-                LOGGER.trace(
-                        "Attempt {}: Need {} {} lock(s) for {}", attempt, keys.size(), shared ? "read" : "write", keys);
-                int acquiredLockCount = 0;
+                if (LOGGER.isTraceEnabled()) {
+                    LOGGER.trace(
+                            "Attempt {}: Acquire {} lock from {}",
+                            attempt,
+                            lockKind,
+                            namedLock.key().name());
+                }
                 try {
                     if (attempt > 1) {
                         Thread.sleep(retryWait);
                     }
-                    for (String key : keys) {
-                        NamedLock namedLock = namedLockFactory.getLock(key);
-                        LOGGER.trace("Acquiring {} lock for '{}'", shared ? "read" : "write", key);
-
-                        boolean locked;
-                        if (shared) {
-                            locked = namedLock.lockShared(time, timeUnit);
-                        } else {
-                            locked = namedLock.lockExclusively(time, timeUnit);
-                        }
-
-                        if (!locked) {
-                            String timeStr = time + " " + timeUnit;
-                            LOGGER.trace(
-                                    "Failed to acquire {} lock for '{}' in {}",
-                                    shared ? "read" : "write",
-                                    key,
-                                    timeStr);
-
-                            namedLock.close();
-                            closeAll();
-                            illegalStateExceptions.add(new IllegalStateException(
-                                    "Attempt " + attempt + ": Could not acquire " + (shared ? "read" : "write")
-                                            + " lock for '" + namedLock.name() + "' in " + timeStr));
-                            break;
-                        } else {
-                            locks.push(namedLock);
-                            acquiredLockCount++;
-                        }
+                    boolean locked;
+                    if (shared) {
+                        locked = namedLock.lockShared(time, timeUnit);
+                    } else {
+                        locked = namedLock.lockExclusively(time, timeUnit);
+                    }
+
+                    if (locked) {
+                        // we are done, get out
+                        locks.push(namedLock);
+                        return;
+                    }
+
+                    // we failed; retry
+                    if (LOGGER.isTraceEnabled()) {
+                        LOGGER.trace(
+                                "Failed to acquire {} lock for '{}' in {}",
+                                lockKind,
+                                namedLock.key().name(),
+                                timeStr);
                     }
                 } catch (InterruptedException e) {
+                    // if we are here, means we were interrupted: fail
+                    close();
                     Thread.currentThread().interrupt();
                     throw new RuntimeException(e);
                 }
-                LOGGER.trace("Attempt {}: Total locks acquired: {}", attempt, acquiredLockCount);
-                if (acquiredLockCount == keys.size()) {
-                    break;
-                }
-            }
-            if (!illegalStateExceptions.isEmpty()) {
-                IllegalStateException ex = new IllegalStateException("Could not acquire lock(s)");
-                illegalStateExceptions.forEach(ex::addSuppressed);
-                throw namedLockFactory.onFailure(ex);
             }
+            // if we are here, means all attempts were unsuccessful: fail
+            close();
+            IllegalStateException ex = new IllegalStateException("Could not acquire " + lockKind + " lock for "
+                    + namedLock.key().resources() + " using lock "
+                    + namedLock.key().name() + " in " + timeStr);
+            throw namedLockFactory.onFailure(ex);
         }
 
-        private void closeAll() {
-            if (locks.isEmpty()) {
-                return;
-            }
-
-            // Release locks in reverse insertion order
-            int released = 0;
+        @Override
+        public void close() {
             while (!locks.isEmpty()) {
                 try (NamedLock namedLock = locks.pop()) {
-                    LOGGER.trace("Releasing {} lock for '{}'", shared ? "read" : "write", namedLock.name());
                     namedLock.unlock();
-                    released++;
+                    if (LOGGER.isTraceEnabled()) {
+                        LOGGER.trace(
+                                "Unlocked and closed {} lock of {}", shared ? "shared" : "exclusive", namedLock.key());
+                    }
                 }
             }
-            LOGGER.trace("Total locks released: {}", released);
-        }
-
-        @Override
-        public void close() {
-            closeAll();
         }
     }
 }

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

@@ -24,6 +24,7 @@
 import org.eclipse.aether.RepositorySystemSession;
 import org.eclipse.aether.artifact.Artifact;
 import org.eclipse.aether.metadata.Metadata;
+import org.eclipse.aether.named.NamedLockKey;
 
 /**
  * Static {@link NameMapper}, always assigns one same name, effectively becoming equivalent to "static" sync context:
@@ -36,15 +37,14 @@ public boolean isFileSystemFriendly() {
     }
 
     @Override
-    public Collection<String> nameLocks(
+    public Collection<NamedLockKey> nameLocks(
             final RepositorySystemSession session,
             final Collection<? extends Artifact> artifacts,
             final Collection<? extends Metadata> metadatas) {
         if (artifacts != null && !artifacts.isEmpty()) {
-            return Collections.singletonList("static-artifact");
+            return Collections.singletonList(NamedLockKey.of("static-artifact"));
         } else if (metadatas != null && !metadatas.isEmpty()) {
-            return Collections.singletonList("static-metadata");
-
+            return Collections.singletonList(NamedLockKey.of("static-metadata"));
         } else {
             return Collections.emptyList();
         }