1
\$\begingroup\$

I've made a small thread pool that i'll be using to communicate with a Redis server:

#include "redox.hpp"

redox::Redox rdx;

void my_cool_function()
{
    // do stuff with rdx
}

int main()
{
    rdx.connect("localhost");
    int max_threads = std::thread::hardware_concurrency();
    std::vector<std::thread> pool;

    for (int i = 0; i < max_threads; i++) {
        pool.push_back(std::thread(my_cool_function));
    }

    for (std::thread &thread : pool) {
        thread.join();
    }

    pool.empty();
    rdx.disconnect();

    return 0;
}

Do I need to call delete rdx on the end of my main function? What about the pool var?

\$\endgroup\$
  • \$\begingroup\$ I think this is not the type of questions you should ask on codereview. Try stackoverflow instead. But rdx and pool have automatic storage duration, so, answer is no, you don't need to. BTW empty doesn't clear the vector, use clear() instead. \$\endgroup\$ – Incomputable May 12 '16 at 17:25
  • \$\begingroup\$ What is automatic storage? \$\endgroup\$ – vinnylinux May 12 '16 at 17:26
  • \$\begingroup\$ When you exit a function, all variables inside are deleted, except those you created with new expression or any other dynamic allocation and static variables. Thus, your pool gets automatically deleted. Lifetime of rdx ends when program terminates, as every global variable and static variable. \$\endgroup\$ – Incomputable May 12 '16 at 17:30
  • 1
    \$\begingroup\$ This question is about creating a thread pool and the code for that has been provided. There are one or two points worth pointing out in an answer so have voted to re-open. \$\endgroup\$ – Martin York May 12 '16 at 23:28
  • \$\begingroup\$ While the question is on-topic now, it could be improved further. Better questions tend to attract better answers. \$\endgroup\$ – Mast May 13 '16 at 0:11
2
\$\begingroup\$

I see a number of things that may help you improve your code.

Make sure to #include all required headers

This program refers to std::vector and std::thread but does not include the corresponding headers. Fix that by adding these lines:

#include <thread>
#include <vector>

Check return values

The function rdx.connect() can fail. Your program should check the return value and handle the case in which it does.

Understand your data structures

The std::vector is used appropriately except for the peculiar call to pool.empty() at the end of the code. The purpose for empty() is to check to see if the vector is empty, not to empty the vector. In any case, it's not necessary to empty it since it will automatically be destroyed when it goes out of scope at the end of main().

Understand the thread pool pattern

The question describes this as a "thread pool" but that's not what I see here. It's not a finite set of threads doing some other number of tasks; rather it's one function being called once for each thread. While this is multithreading, it's not a thread pool. To keep from confusing yourself or others, it's useful to use common terms in the usual way.

Avoid concurrency problems

Any time multiple tasks run in parallel, we have to take extra care to assure that things work correctly. In particular, my_cool_function() claims to "do stuff with rdx", but rdx is a single global variable. The code for that function isn't shown, but it's highly likely that some form of locking will be required to avoid problems with concurrent access to the single global object.

Omit return 0 at the end of main

The compiler will automatically generate a return 0; at the end of main so it is not necessary to supply your own.

\$\endgroup\$
0
\$\begingroup\$

A thread pool keeps a bunch of threads ready for executing work. The emphasis is that after a thread-pool thread finishes a job, it is reused by the pool to execute new jobs. This avoids the overhead of recreating a new thread every time.

Your code simply spawns a few threads that execute one function and then return. Although it might serve your purpose, I wouldn't go as far as calling it a thread pool.

Speaking of purpose, your code doesn't show why you would like to spawn multiple threads to access the Redis server. What exact issue are you trying to solve? I'm not familiar with the Redis API but perhaps it offers async methods that return futures/promises.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.