HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Thread pool on C++11

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
poolthreadstackoverflow

Problem

The class supplies a simple thread pool that uses the thread class from C++11.

When I schedule the job, I pass two std::function objects that the pool will execute: the first function is the real job, the second is executed to send some sort of notification that the job has been done.

I'm not sure if I should merge the two std::function in one and let the user decide if to send the notification or not.

Additionally, not so sure about the naming executeJob vs scheduleJob.

The checking of maxJobsInQueue is not there yet, but when done I cannot decide between throwing exception when the queue is full vs blocking in executeJob until the job can be inserted.

Any other improvement you can suggest?

evr_threadpool.h:

```
#ifndef EVR_THREADPOOL_H
#define EVR_THREADPOOL_H

#include
#include
#include
#include
#include
#include
#include
#include
#include

/**
* @brief The ThreadPool class launches a pre-defined number of threads and
* keeps them ready to execute jobs.
*
* Each job is composed by two functions (std::function):
* the two functions are executed one after another, and the second one
* usually sends some sort of notification that the job has been done.
*
*/
class ThreadPool
{
public:

/**
* @brief Initializes the pool and launches the threads.
*
* @param numThreads number of threads to launch.
* @param maxJobsInQueue
*/
ThreadPool(size_t numThreads, size_t maxJobsInQueue);

/**
* @brief Destructors
*
* Sends a "terminate" signal to the threads and waits for
* their termination.
*
* The threads will complete the currently running job
* before checking for the "terminate" flag.
*/
virtual ~ThreadPool();

/**
* @brief Schedule a job for execution.
*
* The first available thread will pick up the job and run it.
*
* @param job a function that executes the job. It is called
* in t

Solution

A few thoughts:

-
If I understand this correctly, if one of your job functions throws an exception, it ends processing for your entire thread pool; Threads that are running (but did not throw an exception) will keep running, but the ThreadPool object may be deleted as part of stack unwinding. Consider adding another callback to your executeJob similar to std::function on_error.

-
Your executeJob function doesn't actually execute the job - I would rename it to "enqueueJob" or "addJob" or similar.

-
Your Terminated class is a runtime error specialization, but your documentation (or anything in the code really) does not explain why it is a runtime error to have an empty queue.

I assume you use this to stop, not to signal an error state. This may not look like a big thing, but a runtime_error (or specialization) should actually signal an error during the runtime of your applicaition).

For example, the Visual Studio debugger can be set to break automatically when a runtime_error is thrown in the application. Using this implementation would screw up debugging without you meaning to.

Consider either using a second condition variable (or similar) instead of throwing an exception, or adding docs to the source saying "Terminate is a runtime_error specialization because having no jobs to run is an error in this and this case".

Context

StackExchange Code Review Q#36018, answer score: 8

Revisions (0)

No revisions yet.