Cannot be used inside the class

[tuandat64]

I found out that I can't call threadpool
inside class. If I call it in main function in app (like your example) or in dll like this:

void sleep_half_second(const size_t& i, synced_stream* sync_out)
{
    std::this_thread::sleep_for(std::chrono::milliseconds(500));
    sync_out->println("Task ", i, " done.");
}

BOOL APIENTRY DllMain( HMODULE hModule,
                       DWORD  ul_reason_for_call,
                       LPVOID lpReserved
                     )
{
    switch (ul_reason_for_call)
    {
    case DLL_PROCESS_ATTACH:
    case DLL_THREAD_ATTACH:
    case DLL_THREAD_DETACH:
    case DLL_PROCESS_DETACH:
        break;
    }
    thread_pool* pool;
    synced_stream sync_out;
    int i = 1;
    pool = new thread_pool(12);
    pool->push_task(sleep_half_second, i, &sync_out);

    return TRUE;
}

It's OK, doesn't generate error c2064. But if using it in class, it still generates error c2064.

Please help me fix this, I need to call it inside the class

doubleRsi.zip
Thank you very much

[bshoshany]

Like I told you in the other issue you opened, you can't refer to a member function without an object (unless it's a static member function). But you can wrap the member function inside a lambda, for example:

pool.push_task([=] { member_function(arguments); });

You should read this:
https://isocpp.org/wiki/faq/pointers-to-members

This isn't really specific to the thread pool, it's a general thing in C++, so I don't think it's relevant as an issue in this repository. Sorry I couldn't be of more help. Perhaps you should post about this on Stack Overflow?

[tuandat64]

Thank you very much

[tuandat64]

Thank you very much, I have successfully compiled with lambda

	pool->push_task([=] { sleep_half_second(i); });
}

void doubleRsi::sleep_half_second(const size_t& i)
{
	std::this_thread::sleep_for(std::chrono::milliseconds(500));
}

[AdelKS]

How about using std::invoke to support every use case and make everyone happy ? :D

[bshoshany]

Thanks for the suggestion @AdelKS!

Would you be able to explain in more details how you suggest to solve this issue using std::invoke?

As far as I know, in order for std::invoke to call a member function, you have to pass the relevant object to it as well, i.e.:

std::invoke(&class::function, object, arguments);

But the thread pool doesn't know which object it should refer to. This is essentially why the member function needs to be wrapped in a lambda - since the lambda knows which object it was called from.

[AdelKS]

As far as I know, in order for std::invoke to call a member function, you have to pass the relevant object to it as well

Absolutely, and if you decide to support member function through std::invoke, you have to expect that the args... do contain the object in the first position (just like std::invoke expects it)

But the thread pool doesn't know which object it should refer to.

In my use case, I am in the scope where I do have access to this, so I push the task with it :

pool.push_task(&class::function, this, arguments...)

So, for example in the push_task() function you have implemented (the one I am interested in for now), you can write it like this

    template <typename F, typename... A>
    void push_task(const F& task, const A&... args)
    {
        {
            const std::scoped_lock tasks_lock(tasks_mutex);
            tasks.push(std::function<void()>([task, args...] { std::invoke(task, args...); }));
        }
        ++tasks_total;
        task_available_cv.notify_one();
    }

I tried this change on my end and it worked well with member functions. The only thing I am not sure about is if this specific part std::function<void()>([task, args...] { std::invoke(task, args...); } achieves perfect forwarding ?

[AdelKS]

I tried this change on my end and it worked well with member functions. The only thing I am not sure about is if this specific part std::function<void()>([task, args...] { std::invoke(task, args...); } achieves perfect forwarding ?

Probably this would be better ?

    template <typename F, typename... A>
    void push_task(F&& task, A&&... args)
    {
        {
            const std::scoped_lock tasks_lock(tasks_mutex);
            tasks.push(std::function<void()>([task, args...] { std::invoke(task, args...); }));
        }
        ++tasks_total;
        task_available_cv.notify_one();
    }

Although maybe I am missing something on why we souldn't do that x)

[AdelKS]

By digging further into cppreference.com it looks like std::bind is the way to go, so I think something like this is optimal (I saw it in the example given for std::function, then added the "perfect forwarding recipe" xD)

    template <typename F, typename... A>
    void push_task(F&& task, A&&... args)
    {
        {
            const std::scoped_lock tasks_lock(tasks_mutex);
            tasks.push(std::bind(std::forward<F>(task), std::forward<A>(args)...));
        }
        ++tasks_total;
        task_available_cv.notify_one();
    }

[AdelKS]

The changes I proposed in my previous message work! I tested it on this

#include <iostream>
#include <string_view>
#include <string>

#include "BS_thread_pool.hpp"

using namespace std;

class A
{
public:
    void print_val(string added_text)
    {
        cout << a << " " << added_text << endl;
    }

protected:
    int a;
};


int main()
{
    A a;

    BS::thread_pool pool;

    pool.push_task(&A::print_val, a, "some more text");
    pool.wait_for_tasks();

    return 0;
}

PS: It is interesting that it worked with pool.push_task(&A::print_val, a, "some more text"); instead of pool.push_task(&A::print_val, &a, "some more text"); (which works too). std::bind looks to be very versatile.

[bshoshany]

Thanks! I tried this and it indeed works. It also avoids creating a lambda in push_task() if there are arguments, which simplifies it and eliminates the need to distinguish between zero arguments and non-zero arguments. Furthermore, it also works from within the class itself, which is what the original issue was about. Here is an example:

#include "BS_thread_pool.hpp"

BS::synced_stream sync_out;
BS::thread_pool pool;

class test
{
public:
    test(const char* string)
    {
        my_string = string;
    }

    void println(const char* string)
    {
        sync_out.println(string);
    }

    void print_my_string()
    {
        pool.push_task(&test::println, this, my_string);
    }

private:
    const char* my_string = nullptr;
};

int main()
{
    test test_object("Printed from the test object.");
    pool.push_task(&test::println, &test_object, "Printed from main().");
    test_object.print_my_string();
}

So this will definitely go into the next release! However, I need to figure out how to allow this in submit() too.

EDIT: See below for the difference between having a reference on the object in the second argument to push_task() or not.
EDIT 2: Fixed the above test program so it works with all compilers.

[bshoshany]

Okay, I managed to make submit() work as well:

    template <typename F, typename... A, typename R = std::invoke_result_t<std::decay_t<F>, std::decay_t<A>...>>
    [[nodiscard]] std::future<R> submit(F&& task, A&&... args)
    {
        std::function<R()> task_function = std::bind(std::forward<F>(task), std::forward<A>(args)...);
        std::shared_ptr<std::promise<R>> task_promise = std::make_shared<std::promise<R>>();
        push_task(
            [task_function, task_promise]
            {
                try
                {
                    if constexpr (std::is_void_v<R>)
                    {
                        std::invoke(task_function);
                        task_promise->set_value();
                    }
                    else
                    {
                        task_promise->set_value(std::invoke(task_function));
                    }
                }
                catch (...)
                {
                    try
                    {
                        task_promise->set_exception(std::current_exception());
                    }
                    catch (...)
                    {
                    }
                }
            });
        return task_promise->get_future();
    }

Here's the updated test program:

#include "BS_thread_pool.hpp"

BS::synced_stream sync_out;
BS::thread_pool pool;

class test
{
public:
    test(const char* string)
    {
        my_string = string;
    }

    void println(const char* string) const
    {
        sync_out.println(string);
    }

    void print_my_string() const
    {
        pool.push_task(&test::println, this, my_string);
        pool.submit(&test::println, this, my_string).wait();
    }

private:
    const char* my_string = nullptr;
};

int main()
{
    test test_object("Printed from the test object.");
    pool.push_task(&test::println, &test_object, "Printed from main().");
    pool.submit(&test::println, &test_object, "Printed from main().").wait();
    test_object.print_my_string();
}

I will incorporate this into the next release, along with a few other changes I've been working on.

EDIT: Fixed the above test program so it works with all compilers (previously it only worked with GCC).

[bshoshany]

PS: After doing some testing I realized the difference between test_object and &test_object in the second argument. The former will create a copy of the object, while the latter will be a pointer to the actual object. This difference is extremely important when making changes to the object, since any changes made to the copy will not be made in the original object. I corrected the code above to fix that.

[AdelKS]

Awesome, good job ! I will definitely use this great library on my projects :D

[bshoshany]

Glad to hear that! Please feel free to let me know if you have any more suggestions or feature requests :)

[AdelKS]

Dear fellow quantum physicist, do you have a WIP version I could use and give feedback on ? I need the support of member functions so I do not have to update a lot of code x)

[bshoshany]

The new version should be ready within the next few days. Meanwhile, just replace submit() with the version I wrote above and push_task() with:

    template <typename F, typename... A>
    void push_task(F&& task, A&&... args)
    {
        {
            const std::scoped_lock tasks_lock(tasks_mutex);
            tasks.push(std::bind(std::forward<F>(task), std::forward<A>(args)...));
        }
        ++tasks_total;
        task_available_cv.notify_one();
    }

and you'll basically have the new version (at least this part of it - there are a few other new features I'm adding in this version that are unrelated to member functions).

Just one more comment: in my testing, I found out that only GCC accepts a member function without the & prefix as the first argument. Both Clang and MSVC require & to get a function pointer, as well as writing the full name of the function, i.e. &class:function, even when calling it from another member function.

[AdelKS]

Good thanks! I just wanted to confirm that you didn't find some other tweaks/fixes.

Both Clang and MSVC require & to get a function pointer, as well as writing the full name of the function, i.e. &class:function, even when calling it from another member function.

Duely noted !