anthorob
dd1c4dd42f
Replaced unbounded thread pools in `ThreadManager` with bounded `ThreadPoolExecutor` to prevent memory exhaustion under load. Refactored shutdown logic to handle exceptions per-executor, set threads to daemon mode in `BaseThreadFactory`, and aligned synchronization patterns across multiple classes (`TaskManagerImpl`, `ChannelMessagePurger`, and `MusicManager`). Addressed lock cleanup races, double unlock issues, and static collection thread-safety concerns.
99 lines
7.3 KiB
Markdown
99 lines
7.3 KiB
Markdown
---
|
|
sessionId: session-260910-054543-zckm
|
|
---
|
|
|
|
# Requirements
|
|
|
|
### Overview & Goals
|
|
Analyze and fix thread-safety issues and improve the `ExecutorProvider` / `ThreadManager` infrastructure and all code that interacts with it.
|
|
|
|
### Scope
|
|
#### In Scope
|
|
- Fix thread-safety bugs in `ThreadManager`, `TaskManagerImpl`, `ChannelMessagePurger`, `MusicManager`
|
|
- Improve executor pool configuration (bounded pools, proper shutdown)
|
|
- Clean up inconsistent synchronization patterns
|
|
|
|
#### Out of Scope
|
|
- Rewriting the task/command architecture
|
|
- Changing the `QueueManager` synchronization (it's consistent and correct, just verbose)
|
|
|
|
# Technical Design
|
|
|
|
### Current Issues Found
|
|
|
|
#### 1. `ThreadManager` (`core/thread/ThreadManager.java`)
|
|
- **Unbounded cached thread pools** — `newCachedThreadPool()` creates unlimited threads; under load (e.g., many guilds purging simultaneously) this can exhaust memory.
|
|
- **Shutdown bug (line 66-69)** — If `awaitTermination` throws `InterruptedException` on the *first* executor, the `catch` block exits the loop and remaining executors only get `shutdownNow()` without a prior `shutdown()` call. Each executor should be shut down independently.
|
|
- **No daemon threads** — `BaseThreadFactory` doesn't set `thread.setDaemon(true)`, so leaked threads can prevent JVM exit.
|
|
|
|
#### 2. `TaskManagerImpl` (`core/tasks/TaskManagerImpl.java`)
|
|
- **Null values in `ConcurrentHashMap` (line 178-184)** — `computeIfAbsent` returns `null` for `PersistableTask`/`BuiltInTask`, but `ConcurrentHashMap` does **not** permit null values. This will throw `NullPointerException` at runtime.
|
|
- **Mixed synchronization** — `getTasks()` is `synchronized` on `this`, but all mutations use `ConcurrentHashMap` operations (no `synchronized`). The `synchronized` keyword on `getTasks()` provides no actual protection since `insert()`, `removeTask()`, etc. don't synchronize on `this`.
|
|
- **`hasTask()` race (line 93-95)** — Check-then-act across two collections (`tasks` and `pendingTasks`) without synchronization.
|
|
- **`handle()` lock cleanup race (line 191-194)** — After acquiring the lock, it checks `tasks.containsKey()` and removes from `tasksLock`, but another thread could be waiting on the same lock and will find it removed.
|
|
|
|
#### 3. `ChannelMessagePurger` (`utils/ChannelMessagePurger.java`)
|
|
- **Double unlock bug** — `purgeMessages()` acquires `lock` via `tryLock()` (line 68) and releases it in `finally` (line 79). But `safePurge()` also calls `lock.unlock()` (line 154), causing an `IllegalMonitorStateException` or releasing a lock that a *different* thread now holds.
|
|
- **Non-thread-safe static `HashMap`** (line 40) — `instances` is a plain `HashMap` accessed from multiple threads via `computeIfAbsent`. Should be `ConcurrentHashMap`.
|
|
- **Static mutable `ExecutorProvider` binding** — The `volatile` field + `bind()` pattern works but is fragile; a null check race exists between `canLockBeAcquired` callers and `purgeMessages`.
|
|
|
|
#### 4. `MusicManager` (`music/MusicManager.java`)
|
|
- **Inconsistent synchronization** — `isConnected()`, `connect()`, `disconnect()`, `play()` are `synchronized`, but `isCurrentlyPlaying()` (line 114), `getCurrentlyPlaying()` (line 110), `stopTrack()` (line 138), `resume()` (line 142) are not. If these are called from event threads concurrently with `disconnect()`, they can see stale state.
|
|
|
|
#### 5. `BaseThreadFactory` (`core/thread/BaseThreadFactory.java`)
|
|
- Threads are not set as daemon threads, which can prevent clean JVM shutdown if an executor leaks.
|
|
|
|
### Proposed Changes
|
|
|
|
#### `ThreadManager`
|
|
- Replace `newCachedThreadPool()` with `newFixedThreadPool()` or a bounded `ThreadPoolExecutor` with a reasonable max (e.g., `availableProcessors() * 2`).
|
|
- Fix shutdown loop: wrap each executor's `shutdown()`/`awaitTermination()` in its own try-catch so one failure doesn't skip the rest.
|
|
- Consider adding a `getExecutorStatus()` method for observability.
|
|
|
|
#### `BaseThreadFactory`
|
|
- Set `thread.setDaemon(true)` to prevent JVM hang on unclean shutdown.
|
|
|
|
#### `TaskManagerImpl`
|
|
- Fix `computeIfAbsent` null return: use a sentinel or simply don't store an entry for tasks that don't need locking (use `get()` + null check instead).
|
|
- Remove misleading `synchronized` on `getTasks()` or make all task mutations consistently synchronized.
|
|
- Guard `hasTask()` with proper synchronization or accept eventual consistency and document it.
|
|
- Fix lock cleanup race in `handle()` — don't remove from `tasksLock` inside the lock-held section; let `removeTask()` handle cleanup.
|
|
|
|
#### `ChannelMessagePurger`
|
|
- Remove the `lock.unlock()` call on line 154 inside `safePurge()` — the `finally` block in `purgeMessages()` already handles it.
|
|
- Change `instances` from `HashMap` to `ConcurrentHashMap`.
|
|
|
|
#### `MusicManager`
|
|
- Either synchronize all public methods that touch `player`/`AudioManager` state, or remove `synchronized` from methods where JDA already guarantees thread safety and document the threading model.
|
|
|
|
# Delivery Steps
|
|
|
|
### ✓ Step 1: Fix ThreadManager and BaseThreadFactory
|
|
ThreadManager uses bounded pools and has correct shutdown; BaseThreadFactory produces daemon threads.
|
|
|
|
- Replace `Executors.newCachedThreadPool()` in `ThreadManager.getExecutor()` with a bounded `ThreadPoolExecutor` (core=0, max=availableProcessors*2, 60s keepAlive, LinkedBlockingQueue).
|
|
- Fix `stop()` shutdown loop: wrap each executor's `shutdown()`+`awaitTermination()` in its own try-catch so an `InterruptedException` on one executor doesn't skip the rest.
|
|
- In `BaseThreadFactory.newThread()`, add `thread.setDaemon(true)`.
|
|
- Verify the `getScheduledExecutor()` pool size is still appropriate.
|
|
|
|
### ✓ Step 2: Fix TaskManagerImpl thread-safety bugs
|
|
TaskManagerImpl has no null-in-ConcurrentHashMap violations and consistent synchronization.
|
|
|
|
- Replace the `computeIfAbsent` that returns null (line 178-184) with a pattern that avoids storing null: check task type first, only call `computeIfAbsent` when a lock is actually needed.
|
|
- Remove the misleading `synchronized` keyword from `getTasks()`, or switch to a consistent locking strategy across all mutation methods.
|
|
- Fix the lock cleanup race in `handle()`: do not remove from `tasksLock` at line 193; let `removeTask()` be the sole owner of lock cleanup.
|
|
- Document the threading contract for `hasTask()` (check-then-act across two collections is inherently racy).
|
|
|
|
### ✓ Step 3: Fix ChannelMessagePurger concurrency bugs
|
|
ChannelMessagePurger has no double-unlock and uses thread-safe collections.
|
|
|
|
- Remove the redundant `lock.unlock()` call on line 154 in `safePurge()` — the `finally` block in `purgeMessages()` (line 79) already releases the lock.
|
|
- Change the static `instances` field (line 40) from `HashMap` to `ConcurrentHashMap`.
|
|
- Remove the `synchronized` keyword from `countAllChannelMessages()` and `safePurge()` since the `ReentrantLock` already provides mutual exclusion.
|
|
|
|
### ✓ Step 4: Harmonize MusicManager synchronization
|
|
MusicManager has a consistent and documented threading model.
|
|
|
|
- Add `synchronized` to `isCurrentlyPlaying()`, `getCurrentlyPlaying()`, `stopTrack()`, and `resume()` to match the existing pattern on `connect()`/`disconnect()`/`play()`.
|
|
- Alternatively, if JDA's `AudioPlayer` is already thread-safe, remove `synchronized` from all methods and add a class-level Javadoc explaining the threading model.
|
|
- Investigate which approach is correct by checking Lavaplayer's `AudioPlayer` thread-safety guarantees, then apply consistently. |