Introduce ResolverLock, layer on top of low level Named Locks

[cstamas]

The resolver lock adds a thin layer above low lever named locks,
does not leak actualy named lock names, but is keyed by artifact,
hence lock ordering is stable (is based on Artifact not on name mapper).
Finally, resolver lock factory is able to spice up the logic for locking
that is now "hacked in" to not alter SyncContextFactory API.

This is EXPERIMENT for testing reasons only.

[michael-o]

Post 1.7.0.

[gnodet]

AI-Assisted Review — PR #97 (Introduce ResolverLock)

This PR introduces an interesting architectural concept — a ResolverLock layer on top of low-level Named Locks that can dynamically choose shared vs. exclusive locking based on local artifact availability. The idea of upgrading to exclusive locks only when artifacts need downloading is clever and could improve concurrent build performance.

However, this PR has significant challenges that would need to be addressed before it could move forward:

Verified findings

1. Massive staleness & merge conflicts

Created April 25, 2021 (5+ years ago). The merge state is DIRTY/CONFLICTING. Key interfaces have fundamentally diverged — for example, NameMapper on master now returns Collection<NamedLockKey> (with isFileSystemFriendly()) vs. the PR's Collection<String>. NamedLockFactoryAdapter on master is 323 lines with a completely different constructor signature. No CI checks have ever run. The PR would require a complete rewrite against the current codebase.

2. InheritableThreadLocal in SPI module (labeled "HACK")

SyncContextHint.java is placed in maven-resolver-spi and exposes a public static final InheritableThreadLocal<Scope> SCOPE field. Callers in DefaultArtifactResolver and DefaultMetadataResolver use it with explicit // HACK comments. The second commit is titled "Hack more" and the commit message itself states "This is EXPERIMENT for testing reasons only." InheritableThreadLocal as public API in an SPI module is an anti-pattern — it leaks state across thread boundaries unpredictably (especially with thread pools and virtual threads) and breaks encapsulation.

3. TOCTOU race in resolverLocks()

In ResolverLockFactory.resolverLocks(), the sequence is:

1. Get a NamedLock reference (not acquired yet)
2. Check isArtifactAvailable() — queries local repo to decide shared vs. exclusive
3. Create ResolverLock with the shared/exclusive decision baked in
4. Later, tryLock() acquires the lock with the mode decided in step 2

Between steps 2 and 4, another thread could download the artifact, making the lock mode decision stale. A thread could acquire a shared (read) lock when it should have acquired exclusive (write), or vice versa.

4. equals/hashCode based only on key

ResolverLock.equals() compares only key, ignoring requestedShared and effectiveShared. The TreeSet at line 604 uses Comparator.comparing(ResolverLock::key), so entries with identical keys but different effectiveShared values would be silently deduplicated. Combined with the TOCTOU race from finding 3, this could lead to one lock mode decision being silently lost.

Suggestion

The underlying concept (optimizing locking based on artifact availability) has merit, but the current implementation is explicitly experimental. Given the 5+ years of divergence, it would likely be more productive to open a fresh issue capturing the design intent and implement it against the current NamedLockKey-based architecture if there's still interest.

🤖 This review was generated with AI assistance (reviewer + independent verifier pattern). Findings were independently verified before posting.