Fix tasks throwing when queued into the pool while the maximum thread count is reached. #367
Conversation
* Implement a maximum number of threads to exist in the pool. * Implement a threshold to stop creating platform threads and leverage virtual threads instead. * Implement a core size of threads, those of which never leave the pool to buffer inactivity.
* Removed MAX_THREAD_POOL_SIZE and VIRTUAL_THREAD_THRESHOLD, replacing them with MAX_PLATFORM_THREADS and MAX_VIRTUAL_THREADS. * Improve documentation to include @sees for java implementation specifics. Took 7 minutes
…FORM_THREADS and MAX_VIRTUAL_THREADS. * I forgot. Took 4 minutes
…d) upon the thread pool filling. * Use an ArrayBlockingQueue with a capacity of 16 to sit between the decision to make a new thread or wait for an existing one to finish working and take up the task. * Implement a task rejection handler to log the occurrence and execute the task on the caller thread instead of throwing. Took 54 minutes
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe cached thread pool dispatcher was reimplemented with a custom Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CoroutineDispatcher
participant ThreadPoolExecutor
participant PlatformThread
participant VirtualThread
Caller->>CoroutineDispatcher: Dispatch coroutine/task
CoroutineDispatcher->>ThreadPoolExecutor: Submit task
alt Platform threads < MAX_PLATFORM_THREADS
ThreadPoolExecutor->>PlatformThread: Create and run task (daemon)
else
ThreadPoolExecutor->>VirtualThread: Create and run task
end
alt Task queue full
ThreadPoolExecutor->>Caller: Run rejected task in caller thread (CallerRunsPolicy)
ThreadPoolExecutor->>Logger: Log rejection and pool status
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DevMunky
PR: gabber235/Typewriter#357
File: engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt:58-58
Timestamp: 2025-07-10T04:45:22.886Z
Learning: In the Typewriter project's CachedThreadPoolDispatcher.kt, the CORE_POOL_SIZE constant is intentionally set to 6 even though it may exceed MAX_PLATFORM_THREADS on systems with few CPU cores. This is documented behavior where virtual threads will be used in the core pool when MAX_PLATFORM_THREADS < CORE_POOL_SIZE, and should not be flagged as an issue.
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt (1)
Learnt from: DevMunky
PR: #357
File: engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt:58-58
Timestamp: 2025-07-10T04:45:22.886Z
Learning: In the Typewriter project's CachedThreadPoolDispatcher.kt, the CORE_POOL_SIZE constant is intentionally set to 6 even though it may exceed MAX_PLATFORM_THREADS on systems with few CPU cores. This is documented behavior where virtual threads will be used in the core pool when MAX_PLATFORM_THREADS < CORE_POOL_SIZE, and should not be flagged as an issue.
🔇 Additional comments (2)
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt (2)
32-87: Well-structured constants with comprehensive documentation!The constants are properly defined with clear documentation explaining the thread pool behavior, especially the rejection handling strategy and task queue buffering. The
TASK_QUEUE_SIZEof 16 provides an appropriate buffer to prevent immediate task rejection, addressing the core issue mentioned in the PR.
114-121: Rejection handler properly implements the buffering strategy!The custom rejection handler effectively addresses the PR's core objective by logging rejection details and delegating to
CallerRunsPolicyinstead of throwing exceptions. This ensures tasks are executed on the caller's thread when the pool is saturated, preventing task abortion.
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt
Outdated
Show resolved
Hide resolved
engine/engine-core/src/main/kotlin/com/typewritermc/core/utils/CachedThreadPoolDispatcher.kt
Outdated
Show resolved
Hide resolved
* Fix MAX_PLATFORM_THREADS actually being += 1 during runtime (> instead of >=) * Fix the info logging from a rejected task returning an incorrect number of idle threads. Took 8 minutes
|
Looking at this code, would I still be able to spawn 10.000 coroutines in an instant? What would happen? I would like to just have them queued if there is no available thread |
|
Right now, any task that exceeds the limit of the taskQueue and the thread counts gets executed by the caller thread. If we wanted to implement what you are saying, then we would have to create our own mechanism, as the taskQueue filling up is what signifies more threads should be created by the thread pool. |
|
Implemented this but it may need a better approach. Let me know if you are certain we want that functionality, and I will commit it. |
|
Let's do this in two phases. I do want this functionality because executing on the callers thread is going to lead to a ton of subtle bugs which will be basically impossible to diagnose. Simply because it breaks down the implicit contract |
|
Well the bigger question is do you want a task rejection handler that handles too many tasks as they come in, or do you just want to store everything. |
Took 39 minutes
2 participants
Fix issues with tasks being "aborted" (throwing exceptions when queued) upon the thread pool filling.
Use an ArrayBlockingQueue with a capacity of 16 to sit between the decision to make a new thread or wait for an existing one to finish working and take up the task.
Implement a task rejection handler to log the occurrence and execute the task on the caller thread instead of throwing.
Summary by CodeRabbit
Refactor
New Features