scoped thread wrapper for std::thread

Question

I am trying to make a scoped thread.

#include <iostream>
#include <thread>

class ScopedThread {
 public:
    template< class Function, class... Args>
    explicit ScopedThread( int id, Function&& f, Args&&... args)
        : m_thread( std::ref(f), std::forward<Args>(args)...)
        , id(std::move(id)) {
    }



    int getId() const { return id; }

    ~ScopedThread() { m_thread.join(); }
 private:
    std::thread m_thread;
    int id;

};

class Worker {
 public:
    Worker(int id): thd(id, &Worker::work, this) { }

    void work() {
       for( int i = 0; i < 10; i++)
        std::cout << "I am working" << std::endl;
    }


 private:
    ScopedThread thd;
};

int main() {
    Worker(1);
    Worker(2);
    Worker(3);
    Worker(4);
}

When I run the code, it dumps core.

#0  0x00007ffcfbbc6380 in ?? ()
#1  0x00000000004026c9 in std::_Mem_fn<void (Worker::*)()>::operator()<, void>(Worker*) const (this=0x7f0b43551de0, __object=0x7ffcfbbc63c0)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:601
#2  0x00000000004025cd in std::__invoke<void (Worker::*)(), Worker*> (__f=@0x7ffcfbbc6360: (void (Worker::*)(Worker * const)) 0x7ffcfbbc6380, this adjustment 4198629,
    __args=<unknown type in /home/asit/cpp/scope_thd, CU 0x0, DIE 0x2abf>) at /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:247
#3  0x0000000000402532 in std::reference_wrapper<void (Worker::*)()>::operator()<Worker*>(Worker*&&) const (this=0x1b27048,
    __args=<unknown type in /home/asit/cpp/scope_thd, CU 0x0, DIE 0x57fb>) at /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:467
#4  0x00000000004024d2 in std::_Bind_simple<std::reference_wrapper<void (Worker::*)()> (Worker*)>::_M_invoke<0ul>(std::_Index_tuple<0ul>) (this=0x1b27040)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:1731
#5  0x0000000000402485 in std::_Bind_simple<std::reference_wrapper<void (Worker::*)()> (Worker*)>::operator()() (this=0x1b27040)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:1720
#6  0x0000000000402119 in std::thread::_Impl<std::_Bind_simple<std::reference_wrapper<void (Worker::*)()> (Worker*)> >::_M_run() (this=0x1b27028)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/thread:115
#7  0x00007f0b44103a60 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#8  0x00007f0b43920184 in start_thread (arg=0x7f0b43552700) at pthread_create.c:312
#9  0x00007f0b4364d37d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

Can someone tell me how do I forward member functions and arguments to the underlying std::thread class ? I have observed that, the segmentation fault comes only in clang++, not in gcc.

My objective is to make the wrapper class completely replaceable with std::thread class. Wrapper class takes a new argument for thread id.

yes, I guess without -pthread, the error will be quite verbose. Like, Enable multithreading to use std::thread: Operation not permitted — asit_dhal, Jan 25 '17 at 14:41
well, are you sure you are running exactly as pasted here? Because that works for me on gcc 4.8.4 — MK., Jan 25 '17 at 14:45
C++20 is shipped with `std::jthread` for automatic joining. But if you need it right now, why a complex variadic template? Just a `std:: function` argument and pass in a lambda with proper capture list; readable, less confusing with reasonable overhead. — Red.Wave, Jul 31 '20 at 07:24

Vittorio Romeo · Accepted Answer · 2017-01-25T15:06:25.333

9

There are several issues with your implementation of ScopedThread.

There's no need to deal with Function&& f separately. Just handle it as part of the args... pack.

There's no need to move id.

template< class... Args>
explicit ScopedThread( int id, Args&&... args)
    : m_thread( std::forward<Args>(args)...)
    , id(id) {
}

You should make sure that your thread is joinable before calling .join().
```
~ScopedThread() { if(m_thread.joinable()) m_thread.join(); }
```

Applying these changes prevents the segmentation fault on clang++.

The culprit is std::ref(f) - you're creating a temporary reference_wrapper and passing it to std::thread's constructor which uses std::invoke to call it.

According to g++ and UndefinedBehaviorSanitizer:

/usr/local/gcc-head/include/c++/7.0.1/bits/invoke.h:73:46:

runtime error: member call on misaligned address 0x7fff939ec8d3 for type 'struct Worker', which requires 8 byte alignment

The problem is that you're creating a reference to a temporary using std::ref(f), where the temporary is &Worker::work.

Copying f instead of using std::ref doesn't cause any segfault or UndefinedBehaviorSanitizer diagnostic.

edited Jan 25 '17 at 15:06

answered Jan 25 '17 at 14:56

Vittorio Romeo

90,666
33
258
416

so the main thing it seems is to use std::forward(f) instead of std::ref. Can you explain why this was causing the crash? – MK. Jan 25 '17 at 15:05
@MK. Just added an explanation. I think that the issue was that `std::ref(f)` was creating a `reference_wrapper` to a temporary, and then passing it to `std::thread::thread` for invocation. – Vittorio Romeo Jan 25 '17 at 15:07
`if(m_thread.joinable()) m_thread.join();`: when the thread can be already destroyed in this case? aren't members destroyed on leaving destructor? – Andriy Tylychko Jan 25 '17 at 15:15
2

@AndyT: I assumed that *move operations* were eventually going to be implemented for `ScopedThread`. In that case, moving from an `std::thread` instance may leave it non-joinable. – Vittorio Romeo Jan 25 '17 at 15:17
@AndyT no, the order of destruction is ~destructor, then the members. So m_thread will be alive when ~ is executed. – MK. Jan 25 '17 at 15:19
@VittorioRomeo: yeah, makes sense. do you think move semantics is appropriate in this case? wouldn't it be moving *scoped* thread out of scope? – Andriy Tylychko Jan 25 '17 at 15:24
2

@AndyT: I would see it as an "ownership transfer" - think of `unique_ptr`. – Vittorio Romeo Jan 26 '17 at 09:13

Nick · Answer 2 · 2020-07-31T07:34:37.757

My 5 cents for anyone who research the topic.

I did cleanup the code and implemented deleted constructors:

class ScopedThread{
public:
    template<class ...Args>
    explicit ScopedThread(Args &&...args) : thread_(std::forward<Args>(args)...){}

    ScopedThread(ScopedThread &&other){
        thread_ = std::move(other.thread_);
    }

    ScopedThread &operator=(ScopedThread &&other){
        thread_ = std::move(other.thread_);
        return *this;
    }

    std::thread &operator*(){
        return thread_;
    }

    std::thread const &operator*() const{
        return thread_;
    }

    std::thread *operator->(){
        return & operator*();
    }

    std::thread const *operator->() const{
        return & operator*();
    }

    auto get_id() const{
        return thread_.get_id();
    }

    auto join(){
        if (thread_.joinable())
            thread_.join();
    }

    ~ScopedThread(){
        join();
    }

private:
    std::thread thread_;
};

scoped thread wrapper for std::thread

2 Answers2