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\$
7
  • \$\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\$ May 12, 2016 at 17:25
  • \$\begingroup\$ What is automatic storage? \$\endgroup\$
    – vinnylinux
    May 12, 2016 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\$ May 12, 2016 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\$ May 12, 2016 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, 2016 at 0:11

2 Answers 2

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\$
1
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 and acknowledge that you have read and understand our privacy policy and code of conduct.

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