threadpool: fix: min thread amount was ignored on POSIX #122
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #122 +/- ##
==========================================
- Coverage 58.70% 58.64% -0.06%
==========================================
Files 32 32
Lines 2603 2607 +4
Branches 524 526 +2
==========================================
+ Hits 1528 1529 +1
- Misses 741 743 +2
- Partials 334 335 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where the minimum thread count (min_threads) was being ignored in the POSIX threadpool implementation. The fix ensures that when jobs are enqueued, the threadpool creates threads up to the configured minimum if fewer threads currently exist.
Changes:
- Modified
threadpool_create_thread_on_demandto return a boolean indicating success/failure instead of void - Added logic in
threadpool_enqueueto create threads up tomin_threadsbefore processing jobs
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| threadpool_thread_s *thread = (threadpool_thread_s *)calloc(1, sizeof(threadpool_thread_s)); | ||
| thread->handle = handle; | ||
| thread->next = threadpool->threads; | ||
|
|
||
| threadpool->threads = thread; | ||
| threadpool->num_threads++; | ||
| return true; |
There was a problem hiding this comment.
The function creates a pthread but doesn't check if the subsequent calloc fails. If calloc fails (thread is NULL), the code will dereference a NULL pointer on line 172. Additionally, if calloc fails after pthread_create succeeds, the created thread will be orphaned (memory leak) since the thread handle is not stored and cannot be joined later. Add a NULL check after calloc and if it fails, consider detaching the thread or implementing proper cleanup.
| if (threadpool->busy_threads == threadpool->num_threads && threadpool->num_threads < threadpool->max_threads) | ||
| threadpool_create_thread_on_demand(threadpool); |
There was a problem hiding this comment.
The return value of threadpool_create_thread_on_demand is ignored here. If thread creation fails at this point (when all threads are busy), the failure will be silent and the job may not be processed in a timely manner. Consider checking the return value and possibly logging a warning when thread creation fails.
| if (threadpool->busy_threads == threadpool->num_threads && threadpool->num_threads < threadpool->max_threads) | |
| threadpool_create_thread_on_demand(threadpool); | |
| if (threadpool->busy_threads == threadpool->num_threads && threadpool->num_threads < threadpool->max_threads) { | |
| if (!threadpool_create_thread_on_demand(threadpool)) { | |
| fprintf(stderr, "threadpool: warning: failed to create thread on demand; job processing may be delayed\n"); | |
| } | |
| } |
| // Create min amount of threads | ||
| while (threadpool->num_threads < threadpool->min_threads) { | ||
| if (!threadpool_create_thread_on_demand(threadpool)) | ||
| break; | ||
| } |
There was a problem hiding this comment.
The new functionality to ensure minimum threads are created lacks test coverage. Consider adding a test that verifies min_threads are actually created when jobs are enqueued, especially to prevent regression of this bug in the future.
No description provided.