Copilot commented on code in PR #1902:
URL: https://github.com/apache/maven-resolver/pull/1902#discussion_r3369873538
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/concurrency/SmartExecutor.java:
##########
@@ -84,32 +85,44 @@ class Pooled implements SmartExecutor {
@Override
public void submit(Runnable runnable) {
ClassLoader tccl = Thread.currentThread().getContextClassLoader();
- executor.submit(() -> {
- ClassLoader old =
Thread.currentThread().getContextClassLoader();
- Thread.currentThread().setContextClassLoader(tccl);
- try {
- runnable.run();
- } finally {
- Thread.currentThread().setContextClassLoader(old);
- }
- });
+ try {
+ executor.submit(() -> {
+ ClassLoader old =
Thread.currentThread().getContextClassLoader();
+ Thread.currentThread().setContextClassLoader(tccl);
+ try {
+ runnable.run();
+ } finally {
+ Thread.currentThread().setContextClassLoader(old);
+ }
+ });
+ } catch (RejectedExecutionException e) {
+ runnable.run();
+ }
Review Comment:
When task submission is rejected, running the runnable directly will
propagate any RuntimeException/Error to the caller. That changes the effective
semantics from ExecutorService#submit (where the exception is captured by the
Future) and can break callers that expect errors to be reported later (e.g.,
via RunnableErrorForwarder.await()). Consider swallowing the exception here to
preserve the original submit behavior while still ensuring the runnable runs.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/CachingArtifactTypeRegistry.java:
##########
@@ -45,17 +45,18 @@ public static ArtifactTypeRegistry
newInstance(ArtifactTypeRegistry delegate) {
private CachingArtifactTypeRegistry(ArtifactTypeRegistry delegate) {
this.delegate = delegate;
- types = new HashMap<>();
+ types = new ConcurrentHashMap<>();
}
public ArtifactType get(String typeId) {
ArtifactType type = types.get(typeId);
-
- if (type == null) {
- type = delegate.get(typeId);
+ if (type != null) {
+ return type;
+ }
Review Comment:
This cache is now backed by a ConcurrentHashMap, but the
get-then-delegate-then-put sequence can still invoke delegate.get(typeId)
multiple times under contention and doesn't match the PR description that this
would use computeIfAbsent(). Using computeIfAbsent() is simpler and makes the
cache population atomic.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/concurrency/SmartExecutor.java:
##########
@@ -136,13 +149,21 @@ class Limited implements SmartExecutor {
public void submit(Runnable runnable) {
try {
semaphore.acquire();
- executor.submit(() -> {
+ try {
+ executor.submit(() -> {
+ try {
+ runnable.run();
+ } finally {
+ semaphore.release();
+ }
+ });
+ } catch (RejectedExecutionException e) {
try {
runnable.run();
} finally {
semaphore.release();
Review Comment:
If submission is rejected, the fallback executes the runnable on the caller
thread. Any RuntimeException/Error will currently escape submit(), potentially
aborting the scheduling loop and skipping later await()/cleanup logic in
callers. To keep behavior consistent with async submission, catch and swallow
the exception here (the runnable itself can still record/report it, e.g., via
RunnableErrorForwarder).
##########
maven-resolver-generator-gnupg/src/main/java/org/eclipse/aether/generator/gnupg/GnupgSignatureArtifactGenerator.java:
##########
@@ -63,13 +65,13 @@ final class GnupgSignatureArtifactGenerator implements
ArtifactGenerator {
PGPPrivateKey privateKey,
PGPSignatureSubpacketVector hashSubPackets,
String keyInfo) {
- this.artifacts = new ArrayList<>(artifacts);
+ this.artifacts = new CopyOnWriteArrayList<>(artifacts);
this.signableArtifactPredicate = signableArtifactPredicate;
this.secretKey = secretKey;
this.privateKey = privateKey;
this.hashSubPackets = hashSubPackets;
this.keyInfo = keyInfo;
- this.signatureTempFiles = new ArrayList<>();
+ this.signatureTempFiles = new CopyOnWriteArrayList<>();
logger.debug("Created generator using key {}", keyInfo);
Review Comment:
CopyOnWriteArrayList copies the entire backing array on each write. In
generate(), this code performs addAll() and then add() once per signable
artifact, which can become very expensive (CPU+GC) for large deployments. If
thread-safety is needed here, consider a structure optimized for frequent
writes (e.g., synchronized list with snapshots for iteration, or a concurrent
queue), rather than copy-on-write.
--
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]
