gnodet-bot commented on code in PR #2188:
URL: https://github.com/apache/maven-resolver/pull/2188#discussion_r4223016683
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -376,8 +381,8 @@ private static final class CachedPrefixes {
private volatile PrefixTree prefixTree;
private final boolean autoDiscovered;
- private final AtomicBoolean verifyClaimed = new AtomicBoolean(false);
- private volatile String verifiedServedPath;
+ private final Set<String> verifiedServedPaths =
ConcurrentHashMap.newKeySet();
+ private final Set<String> verifiedAbsentPaths =
ConcurrentHashMap.newKeySet();
Review Comment:
⚠️ **Stale Javadoc** — the `CachedPrefixes` class Javadoc just above (lines
372–376) still describes the old single-path model: _"whether verification
happened already, and the denied path (if any) that verification proved the
remote repository actually serves"_. With the new fields (`verifiedServedPaths`
/ `verifiedAbsentPaths`), the semantics changed to per-path caching of both
served and absent paths. Please update the Javadoc accordingly.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -554,28 +562,45 @@ private Result acceptPrefix(RemoteRepository repository,
String path) {
}
boolean accepted = prefixTree.acceptedPath(path);
if (!accepted && cachedPrefixes.autoDiscovered() &&
isVerifyDeniedEnabled(repository)) {
- // synchronized: only the first denial is verified; concurrent
denials wait for the verdict
+ if (cachedPrefixes.isVerifiedServedPath(path)) {
+ return result(
+ true,
+ NAME,
+ "Path " + path + " allowed from " +
repository.getId()
+ + " (verified served despite stale
auto-discovered prefixes)");
+ }
+ if (cachedPrefixes.isVerifiedAbsentPath(path)) {
+ return result(false, NAME, "Path " + path + " NOT allowed
from " + repository.getId());
+ }
synchronized (cachedPrefixes) {
- if (cachedPrefixes.claimVerification() &&
remoteRepositoryServesPath(repository, path)) {
- if (isVerifyDeniedDropsTreeEnabled(repository)) {
- logger.warn(
- "Remote repository {} serves a broken
prefixes file: it denies path {} that the "
- + "repository actually serves;
ignoring auto-discovered prefixes for this "
- + "repository (report this to the
repository administrator)",
- repository.getId(),
- path);
- cachedPrefixes.drop();
- } else {
- logger.warn(
- "Remote repository {} serves path {} that
its auto-discovered prefixes file "
- + "denies; the prefixes file
appears stale. Allowing only this verified "
- + "path; the prefixes filter stays
enforcing for all other paths (set {} "
- + "to true to instead drop the
whole auto-discovered prefixes file; "
- + "report this to the repository
administrator)",
- repository.getId(),
- path,
- CONFIG_PROP_VERIFY_DENIED_DROPS_TREE);
- cachedPrefixes.allowVerifiedServedPath(path);
+ if (cachedPrefixes.prefixTree() == BROKEN) {
+ return noInputResult(repository, "Broken
auto-discovered prefixes dropped");
+ }
+ if (!cachedPrefixes.isVerifiedServedPath(path) &&
!cachedPrefixes.isVerifiedAbsentPath(path)) {
+ VerificationOutcome outcome =
remoteRepositoryServesPath(repository, path);
Review Comment:
💡 **Network call inside `synchronized(cachedPrefixes)`** — the pre-checks
above (lines 563–574) short-circuit for already-verified paths, which is good.
However, for distinct paths arriving concurrently for the same repository,
verification still serializes under this lock. Since the lock is
per-`CachedPrefixes` (per remote repo), this is bounded in practice — but a
short comment explaining the intent (prevent duplicate network calls per path,
not serialize across all paths) would make the reasoning explicit for future
readers.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -376,8 +381,8 @@ private static final class CachedPrefixes {
private volatile PrefixTree prefixTree;
private final boolean autoDiscovered;
- private final AtomicBoolean verifyClaimed = new AtomicBoolean(false);
- private volatile String verifiedServedPath;
+ private final Set<String> verifiedServedPaths =
ConcurrentHashMap.newKeySet();
+ private final Set<String> verifiedAbsentPaths =
ConcurrentHashMap.newKeySet();
Review Comment:
💡 **Wasted allocations on static singletons** — `DISABLED_PREFIXES` and
`NO_INPUT_PREFIXES` are constructed with `autoDiscovered=false`, so these two
`ConcurrentHashMap` instances are allocated at class-load time but will never
be written to (verification only runs when `autoDiscovered` is `true`).
Consider lazy initialisation or a factory constructor that skips allocation for
non-auto-discovered singletons.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
