Fix for ChainedWorkspaceReader

[cstamas]

The readers are immutable, and repository is derived from (immutable) readers, hence, repository is immutable as well.

This PR does not modify API nor behaviour of this class, but removes redundant change detection of immutable readers list.

[gnodet]

Hi @cstamas, thanks for the clean-up! The reasoning is sound — since WorkspaceRepository is immutable and Key captures repository keys at construction time, the lazy-recreation logic in getRepository() is indeed redundant given that the readers list is now fixed at construction time.

A few minor points worth discussing before merging:

1. Null-handling behaviour change (see inline): passing a literal null varargs now throws NPE immediately (previously it was silently treated as an empty list), while individual null elements in the array are now silently dropped (previously they were included and would NPE later). Neither case is part of the documented contract, but it’s a subtle behaviour change that could surprise callers.

2. Undocumented assumption about reader stability (see inline): the correctness of the new code rests on the assumption that WorkspaceReader implementations return a stable WorkspaceRepository (same key) for their entire lifetime. WorkspaceRepository is immutable, which makes this very likely in practice, but WorkspaceReader is an interface. A brief comment capturing this assumption would make the code easier to reason about for future contributors.

3. Minor style (see inline): Collectors.toCollection(ArrayList::new) can be simplified.

4. Tests: there are no unit tests for ChainedWorkspaceReader. A basic test covering construction, findArtifact, findVersions, and getRepository would be a nice addition — especially one that verifies the null-filtering behaviour now that it is an explicit choice.

The @Override additions are good hygiene — thanks for those.

Overall this is a reasonable simplification; just wanted to flag the subtle behaviour differences. Happy to see it land once the above is addressed (or consciously accepted).

— Guillaume (gnodet)

[gnodet]

Nit: note that this slightly changes the null-handling behaviour compared to the previous code. The old Collections.addAll(this.readers, readers) would silently accept a null varargs array (treating it as empty), whereas Arrays.stream(readers) will throw NullPointerException if readers itself is null. On the other hand, individual null elements in the array were previously included (causing an NPE later when iterating), while now they are filtered out. Worth documenting these behaviour changes in Javadoc or a comment, even if null inputs aren’t part of the intended public contract.

[gnodet]

The simplification here hinges on the assumption that WorkspaceReader.getRepository() always returns a repository whose getKey() is stable for the lifetime of the reader. WorkspaceRepository itself is indeed immutable (its key field is final), but WorkspaceReader is an interface — an implementation could theoretically return a different WorkspaceRepository instance with a different key on each invocation. The old code defended against that case by re-checking the key on every call. Would be good to document this assumption, e.g. with a comment like // Assumes WorkspaceReader.getRepository() returns a stable (immutable) repository for the reader’s lifetime.

[gnodet]

Minor style point: since the collected list is immediately wrapped in Collections.unmodifiableList, the exact ArrayList implementation doesn’t matter. You could simplify to .collect(Collectors.toList()), or, since maven-resolver targets Java 11+, even use Stream.of(readers).filter(Objects::nonNull).collect(Collectors.toUnmodifiableList()) (Java 10+).

[cstamas]

@gnodet Windows failed with

Error:  org.eclipse.aether.named.ipc.IpcNamedLockFactoryIT.exclusiveAccess(TestInfo) -- Time elapsed: 25.01 s <<< ERROR!
java.util.concurrent.TimeoutException: exclusiveAccess(org.junit.jupiter.api.TestInfo) timed out after 25 seconds
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
	Suppressed: java.lang.InterruptedException
		at java.base/java.lang.Object.wait0(Native Method)
		at java.base/java.lang.Object.wait(Object.java:389)
		at java.base/java.lang.Thread.join(Thread.java:1887)
		at java.base/java.lang.Thread.join(Thread.java:1963)
		at org.eclipse.aether.named.ipc.NamedLockFactoryTestSupport.exclusiveAccess(NamedLockFactoryTestSupport.java:147)
		at java.base/java.lang.reflect.Method.invoke(Method.java:565)
		... 2 more

See https://github.com/apache/maven-resolver/actions/runs/27139141318/job/80106163823?pr=1909

[gnodet]

The IpcNamedLockFactoryIT.exclusiveAccess() Windows CI failure has been investigated and a fix is up at #1910.

Root cause: When the loser thread's lock acquisition times out in doLockExclusively(), the cleanup call client.unlock(contextId) could throw RuntimeException (e.g., due to Windows-specific socket behavior). This exception propagated out of Access.run() — which only catches InterruptedException — causing the thread to crash without calling loser.countDown(). The winner thread then waited on loser.await() forever, and Surefire killed the JVM at 25 seconds.

A secondary issue: client.unlock() used Long.MAX_VALUE timeout, so if the server/receiver was unresponsive, the thread would hang indefinitely instead of failing fast.