-2

MAJOR EDIT TO SIMPLIFY CODE (and solved)

I would like to be able to make a packaged task that has a free unbound argument, which I will then add at call time of the packaged task.

In this case, I want the first argument to the function (of type size_t) to be unbound.

Here is a working minimal example (this was the solution):

#include <vector>
#include <queue>
#include <memory>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <future>
#include <functional>
#include <stdexcept>
#include <cstdlib>
#include <cstdio>

//REV: I'm trying to "trick" this into for double testfunct( size_t arg1, double arg2), take enqueue( testfunct, 1.0 ), and then internally, execute
// return testfunct( internal_size_t, 1.0 )

template<typename F, typename... Args>
auto enqueue(F&& f, Args&&... args) 
  -> std::future<typename std::result_of<F(size_t, Args...)>::type>
{
  using return_type = typename std::result_of<F(size_t, Args...)>::type;

  //REV: this is where the error was, I was being stupid and thinking this task_contents which would be pushed to the queue should be same (return?) type as original function? Changed to auto and everything worked... (taking into account Jans's result_type(size_t) advice into account.
  //std::function<void(size_t)> task_contents = std::bind( std::forward<F>(f), std::placeholders::_1, std::forward<Args>(args)... );
  auto task_contents = std::bind( std::forward<F>(f), std::placeholders::_1, std::forward<Args>(args)... );

  std::packaged_task<return_type(size_t)> rawtask(
                          task_contents );

  std::future<return_type> res = rawtask.get_future();

  size_t arbitrary_i = 10;
  rawtask(arbitrary_i);
  return res;
}


double testfunct( size_t threadidx, double& arg1 )
{
  fprintf(stdout, "Double %lf Executing on thread %ld\n", arg1, threadidx );
  std::this_thread::sleep_for( std::chrono::milliseconds(1000) );
  return 10; //true;
}

int main()
{
  std::vector<std::future<double>> myfutures;

  for(size_t x=0; x<100; ++x)
    {
      double a=x*10;
      myfutures.push_back(
              enqueue( testfunct, std::ref(a) )
                  );
    }

  for(size_t x=0; x<100; ++x)
    {
      double r = myfutures[x].get();
      fprintf(stdout, "Got %ld: %f\n", x, r );
    }
}
rveale
  • 66
  • 1
  • 10
  • Use a lambda instead, there is no good reason to use `std::bind`. – Matthieu Brucher Jan 05 '19 at 15:22
  • Also your tasks are stored as `void(size_t)`, you cannot decide to change the return type to something else you want. – Matthieu Brucher Jan 05 '19 at 15:31
  • 1
    Can you provide a [mcve], with far more emphasis on minimal? 157 lines of code (that compile!) does not do a good job of presenting the problem you have - which is something that doesn't compile, which is specific to binding. You can probably reduce your problem to fewer than 20 lines of code. Please do so. – Barry Jan 05 '19 at 17:14
  • I will try to edit this to make it a simpler version once I have figured out why it does not compile. It seems perhaps the issue was not the placeholder after all, but rather the return type argument (due to my ignorance, when I added the placeholder, it broke and so that was what I assumed the problem was). – rveale Jan 07 '19 at 12:29
  • I majorly edited code to simplify to minimum example to reproduce my current issue. – rveale Jan 07 '19 at 13:08

2 Answers2

1

The main issues are on ThreadPool::enqueue:

std::function<void(size_t)> task1 = std::bind( std::forward<F>(f), std::placeholders::_1, std::forward<Args>(args)... );

Here, the type of task1 is std::function<void(std::size_t)> but the result of the std::bind when evaluated with funct is convertible to std::function<bool(std::size_t)> and even though as @T.C has pointed out, you can assign the result of the bind to task1, in order to pass task1 to std::make_shared you need to honor the return_type you've got.

Change the above to:

std::function<return_type(size_t)> task1 = std::bind( std::forward<F>(f), std::placeholders::_1, std::forward<Args>(args)... );

Now the same for:

auto task = std::make_shared< std::packaged_task<return_type()> >( task1 );

but in this case is the parameter type that is missing. Change it to:

auto task = std::make_shared< std::packaged_task<return_type(std::size_t)> >( task1 );

ThreadPool::tasks store function objects of type std::function<void(std::size_t)> but you're storing lambda that receive no arguments. Change the tasks.emplace(...) to:

tasks.emplace([task](std::size_t a){ (*task)(a); });
Jans
  • 11,064
  • 3
  • 37
  • 45
  • @T.C. - in which sense? – Jans Jan 05 '19 at 20:39
  • It's perfectly valid to construct a `std::function` from a callable that returns non-void. – T.C. Jan 05 '19 at 20:43
  • Thank you, this solved part of the problem. Yes, the bool(size_t) is a mistake, I left that over from when I was messing around with hacking it to return a bool-type future to temporarily make it work for a demo. I changed the emplace to tasks.emplace([task](std::size_t a){ (*task)(a); });, but now it does not compile with some error about futures invalid use of void expression. However, thanks to your answer I think I am close. I'll try to make a simpler example too, I was just fed up and posted the minimal example that I could get to work without the futures. – rveale Jan 07 '19 at 12:34
  • Jans: Your answer didn't solve the problem, but your advice did, so I'll give you the "solution". I edited the "question" to have the actual solution. – rveale Jan 07 '19 at 13:36
1

The code is not very well formatted, but a solution.

First, you should wrap the results in the lambda creation, not pass functions that can return anything. But if you want to use a shared pointer on a task, this works.

In the prototype:

std::future<void> enqueue(std::function<void(size_t)> f);

using Task = std::function<void(size_t)>;
// the task queue
std::queue<Task> tasks;

std::optional<Task> pop_one();

Implementation becomes:

ThreadPool::ThreadPool(size_t threads)
    :   stop(false)
{
    for(size_t i = 0;i<threads;++i)
        workers.emplace_back(
        [this,i]
            {
                for(;;)
                {
                auto task = pop_one();
                if(task)
                {
                    (*task)(i);
                }
                else break;
                }
            }
        );
}

std::optional<ThreadPool::Task> ThreadPool::pop_one()
{
    std::unique_lock<std::mutex> lock(this->queue_mutex);
    this->condition.wait(lock,
        [this]{ return this->stop || !this->tasks.empty(); });
    if(this->stop && this->tasks.empty())
    {
        return std::optional<Task>();
    }
    auto task = std::move(this->tasks.front()); //REV: this moves into my thread the front of the tasks queue.
    this->tasks.pop();

    return task;
}

template<typename T>
std::future<T> ThreadPool::enqueue(std::function<T(size_t)> fun)
{
    auto task = std::make_shared< std::packaged_task<T(size_t)> >([=](size_t size){return fun(size);});

    auto res = task->get_future();
    {
        std::unique_lock<std::mutex> lock(queue_mutex);

        // don't allow enqueueing after stopping the pool
        if(stop)
        {
            throw std::runtime_error("enqueue on stopped ThreadPool");
        }

        tasks.emplace([=](size_t size){(*task)(size);});
    }
    condition.notify_one();
    return res;
}

And now you can have your main:

int main()
{
  size_t nthreads=3;
  ThreadPool tp(nthreads);
  std::vector<std::future<bool>> myfutures;

  for(size_t x=0; x<100; ++x)
    {
      myfutures.push_back(
          tp.enqueue<bool>([=](size_t threadidx) {return funct(threadidx, (double)x * 10.0);}));
    }

  for(size_t x=0; x<100; ++x)
    {
      bool r = myfutures[x].get();
      std::cout << "Got " << r << "\n";
    }
}

There is now an explicit return type when wrapping the lambda, as the return type is templated.

Matthieu Brucher
  • 21,634
  • 7
  • 38
  • 62
  • This is almost what I want, Jans's answer below provided part of the puzzle (specifically return_type() should include the argument. I do not want to have the user pass a lambda when they call enqueue. it should be an interface almost like std::thread( functionname, arg1, arg2...). The return type will be defined in the future, I assume, so I'm not particularly worried about the templating of enqueue. E.g. if I have function bool funct(size_t arg1, double arg2), if user tries to do e.g. std::future fut = tp.enqueue( funct, 10.0 );, I assume it will break (I have not tried this). – rveale Jan 07 '19 at 12:32
  • You could have an intermediate layer that transforms the lambda into a `std::function` using `return_type()`. Should not be too hard to add that on top of my answer. – Matthieu Brucher Jan 07 '19 at 12:42
  • Could you give me an example? I edited the question to make a simplest example showing the central problem, but it is complaining about void dereferencing when I try to push a lambda containing the packaged_task onto the queue. – rveale Jan 07 '19 at 13:09
  • I tried reusing https://stackoverflow.com/questions/13358672/how-to-convert-a-lambda-to-an-stdfunction-using-templates, but seems like there is still an issue with recognizing the return type :/ – Matthieu Brucher Jan 07 '19 at 14:35
  • Then using a lambda this way: ` auto task_contents = [=](size_t size) { return std::invoke(f, size, std::forward(args)... );}; ` shows that you have a crucial potential bug, your function should not modify its arguments. You should really **COPY** all arguments and have all outputs as... outputs. And fail if someone wants to do a mistake with `std::ref`. – Matthieu Brucher Jan 07 '19 at 14:43
  • HI Matthieu, thank you for the comment. I'm confused. If the user wants his function to modify the arguments, he should be able to...no? That's the whole point of passing by reference or passing a pointer or e.g. some kind of callback. Anyways, I figured out the problem (see the answer) and got everything working. Regarding whether I should specify type carefully, this seems like a design decision. Of course I would like all arguments to be arguments and return values to be return values, but often that is not the case in C++... – rveale Jan 07 '19 at 20:03