Improve thread safety across modules by gnodet · Pull Request #1902 · apache/maven-resolver

gnodet · 2026-06-07T07:37:04Z

Thread Safety Improvements

This PR addresses 31 thread safety issues identified through a systematic audit of the codebase. Each fix is a separate commit for easy review.

HIGH severity fixes (6)

IpcClient close/reconnect lifecycle — reset initialized flag in close() to allow proper reconnection
IpcClient field visibility — make socket/output/input volatile for cross-thread visibility
IpcServer clients HashMap — use ConcurrentHashMap instead of plain HashMap
IpcNamedLock context leak — send unlock on lock timeout to prevent server-side context leak
BasicAuthCache — use ConcurrentHashMap-backed AuthCache to prevent corruption with preemptive auth
SmartExecutor RejectedExecutionException — catch REE to prevent await() hanging forever

MEDIUM severity fixes (15)

DataPool cache initialization — use computeIfAbsent instead of get-then-put race
WeakInternPool.intern() — use compute() for atomic check-and-update
Results.addException/addCycle — synchronize for parallelStream safety
DeferredCredentialsProvider — extend synchronized block to cover delegate read
WagonTransporter close-while-executing — track in-flight wagons for proper cleanup
DefaultProxySelector — use CopyOnWriteArrayList
DefaultMirrorSelector — use CopyOnWriteArrayList
DefaultAuthenticationSelector — use ConcurrentHashMap
SimpleArtifactTypeRegistry — use ConcurrentHashMap
CachingArtifactTypeRegistry — use ConcurrentHashMap.computeIfAbsent()
IpcServer.expirationCheck() — guard against negative Thread.sleep() argument
IpcServer Lock memory leak — remove empty locks from map after unlock
IpcServer Lock.unlock() — complete futures outside monitor to avoid I/O under lock
NamedLockFactorySupport getLock/shutdown — use ReadWriteLock to prevent race
Redisson trySetPermits() — log warning when return value is false

LOW severity fixes (10)

GenericVersionScheme — fix racy cache statistics
Results.errorPath — add volatile
BasicRepositoryConnectorFactory.priority — add volatile
TransferResource.contentLength/resumeOffset — add volatile
Lazy hashCode fields — add volatile in 4 selector/traverser classes
ChainedWorkspaceReader.repository — add volatile
CompositeNamedLock — remove redundant double unlockAll()
WagonTransporter pollWagon — release wagon on reconnect failure
MinioTransporter implPut — close InputStream properly
GnupgSignatureArtifactGenerator — use CopyOnWriteArrayList

Not addressed (4)

F-05 (IpcServer phantom lock on partial multi-key acquisition) — complex architectural change; deferred
F-23 (WebDAV detection redundant OPTIONS) — benign redundancy, accepted
F-28 (DefaultDependencyNode children aliasing) — risk of functional regression
F-34 (File transport TOCTOU) — known limitation of filesystem operations

gnodet · 2026-06-07T09:16:37Z

Remaining Findings Analysis (F-05, F-23, F-28, F-34)

The 4 findings not addressed in this PR were analyzed in detail against the actual source code:

Finding	Severity	Verdict	Rationale
F-05	HIGH	SKIP — False positive	`Lock.unlock()` dual-path logic (holders + waiters removal) under a single monitor already prevents phantom locks. The `thenRun` callback only writes to sockets — it cannot re-add a context to any holders list.
F-23	MEDIUM	SKIP — Benign race	Redundant idempotent OPTIONS requests at worst. The `volatile Boolean` ensures visibility. Synchronizing around an HTTP round-trip would be worse — serializing concurrent PUTs behind network I/O.
F-28	LOW	DEFER	Valid list aliasing through DataPool cache, but intentional for memory optimization. A 2-line defensive copy fix exists if wanted, but no known bug reports and existing transformers already partially defend against it.
F-34	LOW	SKIP — Not a bug	Universal filesystem TOCTOU inherent to all Java NIO. Same `createDirectories` + `newOutputStream` pattern used in 4+ places in the codebase. Standard idiomatic Java.

Full analysis: see resolver-remaining-findings-analysis.md in the audit artifacts.

Copilot

Pull request overview

This PR targets a broad set of concurrency and thread-safety fixes across Maven Resolver modules, primarily by improving cross-thread visibility, replacing non-thread-safe collections, and hardening resource lifecycle handling under concurrent use.

Changes:

Replaced several shared mutable collections with concurrent alternatives (e.g., ConcurrentHashMap, CopyOnWriteArrayList) and tightened atomic cache initialization patterns.
Improved IPC named-lock server/client behavior (lock cleanup, lifecycle reset, avoiding completing futures under monitors).
Hardened transport/executor behavior under shutdown/rejection scenarios and improved resource handling (e.g., ensuring streams are closed).

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
maven-resolver-util/src/main/java/org/eclipse/aether/util/version/GenericVersionScheme.java	Makes version cache hit/miss accounting atomic with cache population.
maven-resolver-util/src/main/java/org/eclipse/aether/util/repository/DefaultProxySelector.java	Uses a thread-safe list for proxy definitions.
maven-resolver-util/src/main/java/org/eclipse/aether/util/repository/DefaultMirrorSelector.java	Uses a thread-safe list for mirror definitions.
maven-resolver-util/src/main/java/org/eclipse/aether/util/repository/DefaultAuthenticationSelector.java	Uses a concurrent map for repository authentication lookup.
maven-resolver-util/src/main/java/org/eclipse/aether/util/repository/ChainedWorkspaceReader.java	Adds `volatile` for cross-thread visibility of repository reference.
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/version/ChainedVersionFilter.java	Makes lazy `hashCode` cache safely publishable across threads.
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/traverser/AndDependencyTraverser.java	Makes lazy `hashCode` cache safely publishable across threads.
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/selector/ExclusionDependencySelector.java	Makes lazy `hashCode` cache safely publishable across threads.
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/selector/AndDependencySelector.java	Makes lazy `hashCode` cache safely publishable across threads.
maven-resolver-util/src/main/java/org/eclipse/aether/util/concurrency/SmartExecutor.java	Adds rejection handling to avoid hangs when submission is refused.
maven-resolver-util/src/main/java/org/eclipse/aether/util/artifact/SimpleArtifactTypeRegistry.java	Uses a concurrent map for artifact type registry storage.
maven-resolver-transport-wagon/src/main/java/org/eclipse/aether/transport/wagon/WagonTransporter.java	Improves wagon lifecycle handling on reconnect failures / transporter close.
maven-resolver-transport-minio/src/main/java/org/eclipse/aether/transport/minio/MinioTransporter.java	Ensures `InputStream` is closed when uploading from an input stream.
maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/DeferredCredentialsProvider.java	Extends synchronization to cover delegate read for credential lookup.
maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java	Introduces a concurrent `AuthCache` implementation and uses it for session caching.
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/NamedLockFactorySupport.java	Rejects lock creation after shutdown during map compute to prevent races.
maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/CompositeNamedLock.java	Removes redundant unlock path on partial acquisition failures.
maven-resolver-named-locks-redisson/src/main/java/org/eclipse/aether/named/redisson/RedissonSemaphoreNamedLockFactory.java	Logs a warning when `trySetPermits()` fails.
maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcServer.java	Uses concurrent client map; avoids completing futures under lock; cleans up empty locks; guards sleep duration.
maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcNamedLock.java	Unlocks context on lock timeout to prevent server-side context leaks.
maven-resolver-named-locks-ipc/src/main/java/org/eclipse/aether/named/ipc/IpcClient.java	Improves cross-thread visibility and resets initialization on close for reconnect.
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegate.java	Synchronizes `Results` mutation/access for parallel collection safety.
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java	Removes get-then-put races via `computeIfAbsent`; makes weak interning atomic.
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/CachingArtifactTypeRegistry.java	Switches internal cache to `ConcurrentHashMap` and refines cache get flow.
maven-resolver-generator-gnupg/src/main/java/org/eclipse/aether/generator/gnupg/GnupgSignatureArtifactGenerator.java	Uses copy-on-write lists for thread-safe artifact/temp-file tracking.
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnectorFactory.java	Makes `priority` safely publishable across threads via `volatile`.
maven-resolver-api/src/main/java/org/eclipse/aether/transfer/TransferResource.java	Adds `volatile` to mutable transfer state for cross-thread visibility.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

gnodet · 2026-06-08T08:57:31Z

+            } catch (RejectedExecutionException e) {
+                runnable.run();
+            }


Fixed. The fallback runnable.run() is now wrapped in a try-catch(RuntimeException | Error) that swallows the exception. This preserves the async submit() semantics where exceptions are captured by the Future. Callers like RunnableErrorForwarder already record the error before re-throwing, so it will still be reported via await().

gnodet · 2026-06-08T08:57:34Z

+                } catch (RejectedExecutionException e) {
                    try {
                        runnable.run();
                    } finally {
                        semaphore.release();


Fixed. Same approach as the Pooled case above — the fallback runnable.run() in Limited.submit(Runnable) now catches and swallows RuntimeException | Error to match async submission semantics. The semaphore is still properly released in the finally block.

gnodet · 2026-06-08T08:57:37Z

    public ArtifactType get(String typeId) {
        ArtifactType type = types.get(typeId);
-
-        if (type == null) {
-            type = delegate.get(typeId);
+        if (type != null) {
+            return type;
+        }


Fixed. Replaced the get-then-delegate-then-put sequence with types.computeIfAbsent(typeId, delegate::get) for atomic cache population. This avoids redundant delegate.get() calls under contention.

gnodet · 2026-06-08T08:57:39Z

+        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);


Fixed. Replaced CopyOnWriteArrayList with plain ArrayList for both artifacts and signatureTempFiles, and made generate() and close() synchronized. This avoids the array-copy overhead on each write while still providing thread safety and happens-before guarantees between the two methods.

cstamas · 2026-06-08T12:19:23Z

    private final RequestTrace trace;

-    private long contentLength = -1L;
+    private volatile long contentLength = -1L;


TransferResource is created by one thread but its contentLength and resumeOffset are set later via setters and read from other threads (e.g. transfer listeners). Without volatile, writes from the producing thread may not be visible to listener threads due to the Java Memory Model — there is no happens-before relationship between the set and get calls across threads.

Agreed, thanks

cstamas · 2026-06-08T12:20:20Z

    private final Map<String, ProvidedChecksumsSource> providedChecksumsSources;

-    private float priority;
+    private volatile float priority;


BasicRepositoryConnectorFactory is a singleton component whose priority can be set via setPriority() and read via getPriority() from different threads. Without volatile, a priority change made by one thread may never be seen by another thread reading it.

Agreed. Perso, I would scrutinize PrioritizedComponents that single user of this field across whole resolver.

cstamas · 2026-06-08T12:21:43Z


    @Override
-    public Collection<? extends Artifact> generate(Collection<? extends Artifact> generatedArtifacts) {
+    public synchronized Collection<? extends Artifact> generate(Collection<? extends Artifact> generatedArtifacts) {


generate() mutates this.artifacts (via addAll) and this.signatureTempFiles. It can be called multiple times during an upload (once per batch of generated artifacts). If two threads invoke it concurrently, the unsynchronized ArrayList operations would cause data races. The synchronized ensures mutual exclusion between generate() and close() which also accesses signatureTempFiles.

While this generic assessment is right, artifact generator instances are created in installer/deployer (on method entry, so if install/deploy happens on two or more thread -- each thread will have own generator), and after use generated artifacts are passed along with existing artifacts to connector, which again, will deploy in parallel.

So, the "If two threads invoke it concurrently" observation is true in general, but I don't see where can it happen.

Change is okay despite my objection. Thanks

cstamas · 2026-06-08T12:26:31Z

-                    return pooled;
-                }
+            Object[] result = new Object[1];
+            synchronized (map) {


The original intern() had a classic check-then-act race: thread A reads map.get(key) and finds null/expired, thread B does the same, then both put a new entry — defeating the purpose of interning (callers get different instances for the same key). Wrapping the check-and-put in synchronized + map.compute() makes the operation atomic. The synchronized block is needed because WeakHashMap is not thread-safe even for single operations.

Agreed, thanks.

cstamas · 2026-06-08T12:26:52Z

        final int maxCycles;

-        String errorPath;
+        volatile String errorPath;


Results is shared across threads during parallel dependency collection. errorPath is written by addException() (now synchronized) but was also read directly by field access. The volatile ensures visibility of writes to threads that read errorPath outside of the synchronized methods.

Agreed, thanks

cstamas · 2026-06-08T12:30:56Z

    private final Set<? extends DependencySelector> selectors;

-    private int hashCode;
+    private volatile int hashCode;


The hashCode() is not concurrency safe here, so I'd make this final instead and modify hashCode() to just return the final int hashCode instead.

Done — made the field final and computed the hashCode eagerly in each constructor. hashCode() now just returns the precomputed value.

cstamas · 2026-06-08T12:30:59Z

    private final Exclusion[] exclusions;

-    private int hashCode;
+    private volatile int hashCode;


The hashCode() is not concurrency safe here, so I'd make this final instead and modify hashCode() to just return the final int hashCode instead.

Done — made the field final and computed the hashCode eagerly in each constructor. hashCode() now just returns the precomputed value.

cstamas · 2026-06-08T12:31:15Z

    private final Set<? extends DependencyTraverser> traversers;

-    private int hashCode;
+    private volatile int hashCode;


The hashCode() is not concurrency safe here, so I'd make this final instead and modify hashCode() to just return the final int hashCode instead.

Done — made the field final and computed the hashCode eagerly in each constructor. hashCode() now just returns the precomputed value.

cstamas · 2026-06-08T12:31:31Z

    private final VersionFilter[] filters;

-    private int hashCode;
+    private volatile int hashCode;


The hashCode() is not concurrency safe here, so I'd make this final instead and modify hashCode() to just return the final int hashCode instead.

Done — made the field final and computed the hashCode eagerly in each constructor. hashCode() now just returns the precomputed value.

cstamas · 2026-06-08T12:34:53Z

    private final List<WorkspaceReader> readers = new ArrayList<>();

-    private WorkspaceRepository repository;
+    private volatile WorkspaceRepository repository;


Isn't this rather final?

You are right — this change has been dropped from this PR. Your PR #1909 handles it properly with AtomicReference and truly final readers.

Thanks — the ChainedWorkspaceReader change has been dropped from this PR since #1909 (now merged) addresses it properly.

cstamas · 2026-06-08T12:36:55Z

Overall looks good, but there are IMHO some unnecessary usage of volatile (not biggie, but anyone seeing this keyword may frantically look for "where is this concurrently used?" and maybe find nothing), and similarly for synchronized. Latter even slows down things in case of true single threaded run, no? (ie obtaining object monitor...)

cstamas · 2026-06-13T20:48:37Z

@gnodet pls review #1909 and then rebase and possibly apply comments?

cstamas · 2026-06-15T18:51:44Z

Pls rebase this @gnodet again?

cstamas · 2026-06-16T09:01:02Z

@gnodet pls sort out conflicts and lets merge

F-02: Declare socket, output, input, and receiver fields as volatile so that writes in close(Throwable) are visible to concurrent readers in send() and receive() without requiring synchronization on this.

F-03: Replace plain HashMap with ConcurrentHashMap for the clients map so that close() and expirationCheck() can safely access it without synchronization, preventing ConcurrentModificationException and data races.

…d AuthCache F-06: Replace BasicAuthCache (backed by plain HashMap) with a ConcurrentAuthCache backed by ConcurrentHashMap, preventing data corruption when concurrent requests call authCache.put() during preemptive auth.

F-13: Replace plain ArrayList with CopyOnWriteArrayList to prevent ConcurrentModificationException when add() is called concurrently with getProxy() iteration.

F-14: Replace plain ArrayList with CopyOnWriteArrayList to prevent ConcurrentModificationException when add() is called concurrently with getMirror()/findMirror() iteration.

F-15: Replace plain HashMap with ConcurrentHashMap to prevent data corruption when add() and getAuthentication() are called from different threads.

F-16: Replace plain HashMap with ConcurrentHashMap to ensure visibility of types populated on the main thread to worker threads querying via the session.

F-17: Replace plain HashMap with ConcurrentHashMap to prevent corruption when concurrent parallel stream workers query the artifact type registry simultaneously.

F-18: Clamp Thread.sleep() argument to minimum 1ms to prevent IllegalArgumentException when left goes negative while clients are still connected. Previously this silently killed the expiration thread, causing the server to never shut down.

F-19: Remove Lock entries from the locks ConcurrentHashMap when holders and waiters are both empty after unlock, preventing unbounded accumulation over the server's lifetime.

F-20: Collect futures to complete during the synchronized block but complete them after releasing the Lock monitor, preventing I/O operations (socket writes in thenRun callbacks) from being serialized under the lock.

F-21: Re-check shutdown flag inside the locks.compute() lambda to prevent creating locks on a backend that was shut down between the initial check and the actual lock creation.

F-22: Check the return value of trySetPermits(). If it returns false (e.g., a crashed process left the semaphore in a depleted state), log a warning instead of silently proceeding with a potentially broken semaphore.

F-24: Use a single computeIfAbsent() call instead of separate get() then computeIfAbsent(), and derive hit/miss statistics from whether the mapping function was invoked.

F-25: Make errorPath volatile to ensure cross-thread visibility when set from parallel stream workers and read from the main thread.

F-26: Make the priority field volatile to ensure cross-thread visibility between setPriority() and getPriority() calls.

F-27: Make contentLength and resumeOffset volatile to ensure visibility when written on worker threads and read by monitoring or logging threads.

F-29: Make cached hashCode fields volatile in ExclusionDependencySelector, AndDependencySelector, AndDependencyTraverser, and ChainedVersionFilter to ensure visibility across threads.

F-31: Return false immediately after unlockAll() on lock failure instead of breaking out of the loop and calling unlockAll() again on the already-empty deque.

…nstead of re-queuing

F-33: Wrap task.newInputStream() in try-with-resources so the stream is properly closed after Files.copy(), preventing resource leaks on both normal and error paths.

F-35: Replace plain ArrayList with CopyOnWriteArrayList for signatureTempFiles as a defensive measure against potential concurrent access between generate() and close().

…le overloads Pooled.submit(Callable) and Limited.submit(Callable) did not catch RejectedExecutionException, unlike their Runnable counterparts. On rejection, Pooled would let the REE propagate with a never-completed future, and Limited would additionally leak a semaphore permit. Apply the same try-catch-fallback pattern: on rejection, run the callable on the caller thread and complete the future inline.

The unlock() path had a race: after checking l.isEmpty(), another thread could computeIfAbsent and re-use the same Lock object, then the remove(l.key, l) would remove a Lock that now has holders. Replace the two-step isEmpty() check + remove() with a single locks.compute() call that atomically checks emptiness and removes.

Collections.synchronizedMap does not make computeIfAbsent atomic; the default Map.computeIfAbsent implementation performs a non-atomic get-then-put sequence, risking concurrent structural modification of the underlying WeakHashMap. Wrap the computeIfAbsent call in synchronized(versionCache) to use the same monitor that synchronizedMap uses internally.

Collections.synchronizedMap does not make compute() atomic; the default Map.compute implementation performs a non-atomic read-modify sequence, risking concurrent structural modification of the underlying WeakHashMap. Wrap the compute() call in synchronized(map) to use the same monitor that synchronizedMap uses internally.

The write paths addException() and addCycle() are synchronized but getResult() and getErrorPath() were not, so readers could observe stale data without a happens-before edge from the writer. Make both getter methods synchronized to establish proper visibility guarantees for cross-thread reads.

…tifacts The signatureTempFiles field was changed to CopyOnWriteArrayList for thread safety, but the artifacts field (also mutated via addAll in generate()) was left as a plain ArrayList. Change artifacts to CopyOnWriteArrayList for consistency, since both fields are mutated and iterated in the same methods.

After #1902 only one had changes, but they always used together. Also, rather make `close()` idempotent than synchronized.

cstamas added this to the 2.0.19 milestone Jun 7, 2026

cstamas added the enhancement New feature or request label Jun 7, 2026

gnodet marked this pull request as ready for review June 7, 2026 12:19

slachiewicz requested a review from Copilot June 7, 2026 18:23

slachiewicz approved these changes Jun 7, 2026

View reviewed changes

Copilot started reviewing on behalf of slachiewicz June 7, 2026 18:24 View session

Copilot AI reviewed Jun 7, 2026

View reviewed changes

cstamas reviewed Jun 8, 2026

View reviewed changes

jprinet mentioned this pull request Jun 12, 2026

HttpTransporter shares AuthCache in the session cache under a classloader-agnostic key -> ClassCastException across ClassRealms #1919

Closed

cstamas requested a review from slawekjaranowski June 13, 2026 20:51

gnodet force-pushed the gnodet/thread-safety-fixes branch from 8ecda11 to 1fcad31 Compare June 13, 2026 23:50

cstamas approved these changes Jun 16, 2026

View reviewed changes

gnodet added 3 commits June 16, 2026 09:12

Fix IpcClient field visibility — make socket/output/input volatile

ae2750c

F-02: Declare socket, output, input, and receiver fields as volatile so that writes in close(Throwable) are visible to concurrent readers in send() and receive() without requiring synchronization on this.

Fix IpcServer clients HashMap — use ConcurrentHashMap

b06348b

F-03: Replace plain HashMap with ConcurrentHashMap for the clients map so that close() and expirationCheck() can safely access it without synchronization, preventing ConcurrentModificationException and data races.

gnodet force-pushed the gnodet/thread-safety-fixes branch from 27e32d6 to f4fc2bb Compare June 16, 2026 09:14

gnodet added 25 commits June 16, 2026 09:19

Fix DefaultProxySelector — use CopyOnWriteArrayList for proxies

3e718c1

F-13: Replace plain ArrayList with CopyOnWriteArrayList to prevent ConcurrentModificationException when add() is called concurrently with getProxy() iteration.

Fix DefaultMirrorSelector — use CopyOnWriteArrayList for mirrors

997c9f7

F-14: Replace plain ArrayList with CopyOnWriteArrayList to prevent ConcurrentModificationException when add() is called concurrently with getMirror()/findMirror() iteration.

Fix DefaultAuthenticationSelector — use ConcurrentHashMap

968e157

F-15: Replace plain HashMap with ConcurrentHashMap to prevent data corruption when add() and getAuthentication() are called from different threads.

Fix SimpleArtifactTypeRegistry — use ConcurrentHashMap

cbe642e

F-16: Replace plain HashMap with ConcurrentHashMap to ensure visibility of types populated on the main thread to worker threads querying via the session.

Fix CachingArtifactTypeRegistry — use ConcurrentHashMap

cc21a5f

F-17: Replace plain HashMap with ConcurrentHashMap to prevent corruption when concurrent parallel stream workers query the artifact type registry simultaneously.

Fix IpcServer Lock memory leak — remove empty locks from map

957eee5

F-19: Remove Lock entries from the locks ConcurrentHashMap when holders and waiters are both empty after unlock, preventing unbounded accumulation over the server's lifetime.

Fix IpcServer Lock.unlock() — complete futures outside monitor

6942ac9

F-20: Collect futures to complete during the synchronized block but complete them after releasing the Lock monitor, preventing I/O operations (socket writes in thenRun callbacks) from being serialized under the lock.

Fix NamedLockFactorySupport getLock/shutdown race

d3bf1c6

F-21: Re-check shutdown flag inside the locks.compute() lambda to prevent creating locks on a backend that was shut down between the initial check and the actual lock creation.

Fix Redisson trySetPermits() — log warning on failure

53d984c

F-22: Check the return value of trySetPermits(). If it returns false (e.g., a crashed process left the semaphore in a depleted state), log a warning instead of silently proceeding with a potentially broken semaphore.

Fix GenericVersionScheme racy cache statistics

5902f99

F-24: Use a single computeIfAbsent() call instead of separate get() then computeIfAbsent(), and derive hit/miss statistics from whether the mapping function was invoked.

Fix Results.errorPath visibility — add volatile

09a975c

F-25: Make errorPath volatile to ensure cross-thread visibility when set from parallel stream workers and read from the main thread.

Fix BasicRepositoryConnectorFactory.priority — add volatile

ff8b18b

F-26: Make the priority field volatile to ensure cross-thread visibility between setPriority() and getPriority() calls.

Fix TransferResource field visibility — add volatile

d498276

F-27: Make contentLength and resumeOffset volatile to ensure visibility when written on worker threads and read by monitoring or logging threads.

Fix lazy hashCode fields — add volatile in 4 classes

109a7d8

F-29: Make cached hashCode fields volatile in ExclusionDependencySelector, AndDependencySelector, AndDependencyTraverser, and ChainedVersionFilter to ensure visibility across threads.

Fix CompositeNamedLock — remove redundant double unlockAll

d74d0f0

F-31: Return false immediately after unlockAll() on lock failure instead of breaking out of the loop and calling unlockAll() again on the already-empty deque.

Fix WagonTransporter pollWagon — release wagon on reconnect failure i…

372b971

…nstead of re-queuing

Fix MinioTransporter implPut — close InputStream from task

0506686

F-33: Wrap task.newInputStream() in try-with-resources so the stream is properly closed after Files.copy(), preventing resource leaks on both normal and error paths.

Fix GnupgSignatureArtifactGenerator — use CopyOnWriteArrayList

aad1ca4

F-35: Replace plain ArrayList with CopyOnWriteArrayList for signatureTempFiles as a defensive measure against potential concurrent access between generate() and close().

gnodet force-pushed the gnodet/thread-safety-fixes branch from f4fc2bb to 80c4eb1 Compare June 16, 2026 09:21

gnodet merged commit e66df50 into apache:master Jun 16, 2026
14 checks passed

github-actions Bot assigned gnodet Jun 16, 2026

cstamas mentioned this pull request Jun 16, 2026

Align generators #1929

Merged

cstamas added a commit that referenced this pull request Jun 16, 2026

Align generators (#1929)

35d0d46

After #1902 only one had changes, but they always used together. Also, rather make `close()` idempotent than synchronized.

Conversation

gnodet commented Jun 7, 2026

Thread Safety Improvements

HIGH severity fixes (6)

MEDIUM severity fixes (15)

LOW severity fixes (10)

Not addressed (4)

Uh oh!

gnodet commented Jun 7, 2026

Remaining Findings Analysis (F-05, F-23, F-28, F-34)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!