--- name: check-concurrency description: Find concurrency issues including race conditions, deadlocks, unsafe shared state, and improper synchronization --- # Check Concurrency Skill Identify concurrency bugs, race conditions, deadlocks, and thread safety issues in the codebase. ## Instructions ### 1. Identify Shared Mutable State Search for mutable state that may be accessed by multiple threads: **Instance/Static Fields**: ```java // Dangerous - mutable shared state private Map cache = new HashMap<>(); private List items = new ArrayList<>(); private int counter = 0; private boolean flag = false; // Safe alternatives private final ConcurrentHashMap cache = new ConcurrentHashMap<>(); private final CopyOnWriteArrayList items = new CopyOnWriteArrayList<>(); private final AtomicInteger counter = new AtomicInteger(); private final AtomicBoolean flag = new AtomicBoolean(); ``` Use `Grep` to find: ``` private.*Map< private.*List< private.*Set< private static(?!.*final) private.*= 0; private.*= false; private.*= true; private.*= null; ``` ### 2. Check for Race Conditions **Check-Then-Act Anti-patterns**: ```java // RACE CONDITION: check and act not atomic if (!map.containsKey(key)) { map.put(key, value); // Another thread may have put between check and act } // SAFE: Use atomic operations map.putIfAbsent(key, value); map.computeIfAbsent(key, k -> createValue()); ``` **Read-Modify-Write Anti-patterns**: ```java // RACE CONDITION: increment not atomic counter++; counter = counter + 1; // SAFE: Use atomic types atomicCounter.incrementAndGet(); ``` **Lazy Initialization**: ```java // RACE CONDITION: double-checked locking without volatile if (instance == null) { synchronized(lock) { if (instance == null) { instance = new Instance(); // May return partially constructed } } } // SAFE: Use volatile or holder pattern private static volatile Instance instance; // or private static class Holder { static final Instance INSTANCE = new Instance(); } ``` ### 3. Detect Potential Deadlocks **Lock Ordering Issues**: ```java // DEADLOCK RISK: Different lock ordering // Thread 1: synchronized(lockA) { synchronized(lockB) { ... } } // Thread 2: synchronized(lockB) { synchronized(lockA) { ... } } ``` Search for nested synchronization: ```java synchronized.*\{[\s\S]*synchronized ``` **Resource Acquisition**: ```java // DEADLOCK RISK: Acquiring multiple semaphores semaphoreA.acquire(); semaphoreB.acquire(); // What if another thread holds B, wants A? ``` ### 4. Analyze Synchronization Usage **Over-synchronization** (performance issue): ```java // BAD: Synchronized on entire method when only part needs it public synchronized void process(Data data) { // ... lots of non-shared work ... cache.put(key, value); // Only this needs sync } // BETTER: Minimize synchronized scope public void process(Data data) { // ... non-shared work ... synchronized(lock) { cache.put(key, value); } } ``` **Under-synchronization** (correctness issue): ```java // BAD: Some methods synchronized, some not public synchronized void add(T item) { list.add(item); } public T get(int i) { return list.get(i); } // NOT synchronized! ``` ### 5. Check Thread-Safe Collections Usage **Compound Operations on Concurrent Collections**: ```java ConcurrentHashMap map = ...; // RACE CONDITION: Compound operation not atomic V value = map.get(key); if (value == null) { value = computeValue(); map.put(key, value); // Another thread may have computed too } // SAFE: Use atomic compute methods V value = map.computeIfAbsent(key, k -> computeValue()); ``` **Iteration Safety**: ```java // UNSAFE: ConcurrentModificationException or stale data for (K key : map.keySet()) { map.remove(key); // Modifying during iteration } // SAFE: Use iterator.remove() or collect keys first ``` ### 6. Virtual Threads & StructuredTaskScope (Java 21+) This project uses Java 24 virtual threads. Check for: **Pinning Issues**: ```java // BAD: synchronized pins virtual thread to carrier synchronized(lock) { blockingCall(); // Virtual thread pinned! } // BETTER: Use ReentrantLock lock.lock(); try { blockingCall(); // Virtual thread can unmount } finally { lock.unlock(); } ``` **StructuredTaskScope Usage**: ```java // Check for proper scope management try (var scope = new StructuredTaskScope.ShutdownOnFailure()) { var future1 = scope.fork(() -> task1()); var future2 = scope.fork(() -> task2()); scope.join(); // Must join before getting results scope.throwIfFailed(); // Must check for failures return combine(future1.get(), future2.get()); } // ISSUES TO CHECK: // - Missing join() before accessing results // - Missing throwIfFailed() for ShutdownOnFailure // - Scope not closed (try-with-resources required) // - Sharing scope between threads ``` ### 7. Reactive/WebFlux Concurrency **Publisher Sharing**: ```java // UNSAFE: Sharing mutable state in reactive chain List results = new ArrayList<>(); // Shared mutable! flux.doOnNext(item -> results.add(item)) // Race condition! .subscribe(); // SAFE: Use collectList() or reduce() flux.collectList().subscribe(results -> ...); ``` **Scheduler Awareness**: ```java // Check that shared state access considers scheduler mono.publishOn(Schedulers.parallel()) // Multiple threads! .map(data -> { sharedState.update(data); // RACE CONDITION return data; }); ``` ### 8. This Project's Specific Patterns **Files to examine**: - `AggregatorService.java` - Uses StructuredTaskScope, Semaphores, ConcurrentHashMap - `CurrentConditionsService.java` - Caching with concurrent access - `ForecastService.java` - Data parsing with potential shared state - `*Strategy.java` - Strategy implementations called concurrently **Known concurrent structures in this project**: ```java // Check these are used correctly: ConcurrentHashMap forecastCache ConcurrentHashMap currentConditions AtomicReference> spots Semaphore (for rate limiting) ``` ### 9. Common Concurrency Anti-patterns | Pattern | Issue | Fix | |---------|-------|-----| | `HashMap` in concurrent context | Race condition | Use `ConcurrentHashMap` | | `ArrayList` shared between threads | Race condition | Use `CopyOnWriteArrayList` or synchronize | | `SimpleDateFormat` shared | Not thread-safe | Use `DateTimeFormatter` (immutable) | | Non-volatile field read by multiple threads | Visibility issue | Use `volatile` or atomic | | `synchronized(this)` | Lock on public object | Use private lock object | | Catching `InterruptedException` silently | Lost interrupt | Re-interrupt or propagate | ## Output Format ```markdown ## Concurrency Analysis Report ### Summary | Category | Issues Found | Severity | |----------|--------------|----------| | Race Conditions | X | Critical | | Potential Deadlocks | X | Critical | | Unsafe Shared State | X | High | | Synchronization Issues | X | Medium | | Virtual Thread Issues | X | Medium | ### Critical Issues #### Race Condition: [Description] **File**: `path/to/file.java:line` **Pattern**: Check-then-act on HashMap **Threads Involved**: Scheduler thread, Request threads ```java // Current code if (!cache.containsKey(key)) { cache.put(key, expensiveCompute()); } ``` **Risk**: Duplicate computation, inconsistent state **Fix**: ```java cache.computeIfAbsent(key, k -> expensiveCompute()); ``` #### Potential Deadlock: [Description] **File**: `path/to/file.java:line` **Pattern**: Nested locks with inconsistent ordering **Fix**: Establish global lock ordering or use tryLock with timeout ### High Priority Issues #### Unsafe Shared State | File | Line | Field | Issue | Fix | |------|------|-------|-------|-----| | Service.java | 15 | `Map cache` | Non-concurrent map | Use ConcurrentHashMap | ### Medium Priority Issues #### Synchronization Improvements - `File.java:42` - Consider narrowing synchronized scope - `File.java:78` - Missing volatile on field read by multiple threads ### Verified Thread-Safe Patterns | File | Pattern | Why It's Safe | |------|---------|---------------| | AggregatorService.java | ConcurrentHashMap | Proper atomic operations used | | AggregatorService.java | Semaphore | Proper acquire/release in try-finally | ### StructuredTaskScope Analysis | Location | Usage | Status | |----------|-------|--------| | AggregatorService:120 | ShutdownOnFailure | ✓ Correct | | AggregatorService:150 | fork/join | ✓ Proper ordering | ### Recommendations 1. **Immediate**: Fix race condition in `Cache.java:42` 2. **Review**: Audit all HashMap usages for thread safety 3. **Consider**: Add @ThreadSafe/@NotThreadSafe annotations for documentation 4. **Testing**: Add concurrent stress tests for critical sections ``` ## Execution Steps 1. Use `Grep` to find mutable field declarations 2. Use `Grep` to find `synchronized`, `Lock`, `Semaphore` usage 3. Use `Grep` to find concurrent collection usage 4. Read files to analyze compound operations 5. Check StructuredTaskScope for proper join/close 6. Look for check-then-act and read-modify-write patterns 7. Analyze lock ordering for deadlock potential 8. Generate categorized report ## Notes - Virtual threads change some concurrency patterns (synchronized pins carrier thread) - ConcurrentHashMap is safe for individual operations, not compound ones - AtomicReference doesn't make the referenced object thread-safe - Reactive chains may execute on different threads at different stages - `@Scheduled` methods may run concurrently if previous execution is slow - Focus on request-handling code paths over initialization code