1
\$\begingroup\$

I wrote this simple scoped thread to use it as a class member to ensure the the thread running on a class method doesn't continue running after the class has been destructed. Also to spawn threads from the main() function to ensure that I don't forget any running threads upon exit .

This can be achieved by calling join() method of the C++11 std::thread but I wanted a class that will automatically do this.

I didn't implement detach() function because I think this kills the purpose of a scoped thread; is this right?

I'd like to know if there is any bug, memory leak, other errors ...

#include <tuple>
#include <process.h>

extern "C"
{

    __declspec(dllimport)
        BOOL 
        __stdcall
        CloseHandle(
            HANDLE hObject
        );

    __declspec(dllimport)
        DWORD
        __stdcall
        WaitForSingleObject(
            HANDLE hHandle,
            DWORD dwMilliseconds
        );

    __declspec(dllimport)
        DWORD
        __stdcall
        GetCurrentThreadId(
            void
        );
}
template <class T, auto CloseFn>
struct HandleDeleter
{
    using pointer = T;
    void operator ()(pointer handle) { CloseFn(handle); }
};

using Handle = std::unique_ptr<void, HandleDeleter<HANDLE, CloseHandle>>;

inline std::error_code make_system_error(DWORD val) noexcept
{
    return std::error_code{ static_cast<int>(val), std::system_category() };
}

inline std::error_code make_system_error(int val) noexcept
{
    return std::error_code{ val, std::system_category() };
}


namespace LIB_NAMESPACE
{


namespace detail
{
    template <class Func, class ... Args>
    struct ThreadParams
    {
    public:

        ThreadParams(Func&& func, Args && ... args)
            : fn{ std::forward<Func>(func) }, args_tuple{ std::forward<Args>(args)... } 
        {}

        void Invoke()
        {
            std::apply(fn, args_tuple);
        }

    private:
        Func fn;
        std::tuple<Args...> args_tuple;
    };

    template <class ParamsType>
    unsigned __stdcall ThreadFunc(void* args)
    {
        auto params = reinterpret_cast<ParamsType*>(args);
        params->Invoke();
        delete params;
        return 0;
    }

}

class scoped_thread
{
public:

    using native_handle_type = Handle;
    struct thread_id
    {
    public:

        friend class scoped_thread;

        thread_id() = default;

        thread_id(unsigned int id) : id_{ id } {}

        thread_id(const thread_id&) = default;

        thread_id(thread_id&& other) noexcept : id_{ other.id_ }
        {
            other.invalidate();
        }

        thread_id& operator=(const thread_id&) = default;

        thread_id& operator=(thread_id&& other) noexcept
        {
            id_ = other.id_;
            other.invalidate();
            return *this;
        }

        ~thread_id()
        {
            id_ = 0;
        }

        unsigned int id() const { return id_; }

        bool operator==(const thread_id& other) const
        {
            return id() == other.id();
        }

        bool operator!=(const thread_id& other) const
        {
            return id() != other.id();
        }

        bool operator>(const thread_id& other) const
        {
            return id() > other.id();
        }

        bool operator>=(const thread_id& other) const
        {
            return id() >= other.id();
        }

        bool operator<(const thread_id& other) const
        {
            return id() < other.id();
        }

        bool operator<=(const thread_id& other) const
        {
            return id() <= other.id();
        }

    private:

        void invalidate() { id_ = 0; }

        unsigned int * id_ptr() { return &id_; }

        unsigned int id_ = 0;
    };

    scoped_thread() = default;

    template <class Fn, class ... Args>
    scoped_thread(Fn&& fn, Args && ... args)
    {
        using ParamsType = detail::ThreadParams<Fn, Args...>;
        ParamsType* params = new ParamsType{ std::forward<Fn>(fn), std::forward<Args>(args)... };
        thd_handle.reset(
            reinterpret_cast<void*>(_beginthreadex(nullptr, 0,
                detail::ThreadFunc<ParamsType>, params, 0, id.id_ptr()))
        );
        if (!thd_handle)
        {
            delete params;
            throw std::system_error(errno, std::system_category());
        }
    }

    scoped_thread(scoped_thread&&) = default;

    scoped_thread& operator=(scoped_thread&& other) noexcept
    {
        if (joinable())
            std::terminate();
        this->~scoped_thread();
        new (this) scoped_thread{ std::move(other) };
        return *this;
    }

    native_handle_type& native_handle() noexcept { return thd_handle; }

    const native_handle_type& native_handle() const noexcept { return thd_handle; }

    thread_id get_id() const noexcept { return id; }

    bool joinable() const noexcept
    {
        return get_id() != thread_id{};
    }

    void join()
    {
        check_if_valid_join();
        WaitForSingleObject(thd_handle.get(), std::numeric_limits<DWORD>::max());
        thd_handle.reset();
        id.invalidate();
    }

    ~scoped_thread()
    {
        if (joinable())
            join();
    }

private:

    void check_if_valid_join()
    {
        if (!thd_handle)
            throw std::system_error(std::make_error_code(std::errc::no_such_process));
        if (get_id() == GetCurrentThreadId())
            throw std::system_error(std::make_error_code(std::errc::resource_deadlock_would_occur));
        if (!joinable())
            throw std::system_error(std::make_error_code(std::errc::invalid_argument));
    }

    native_handle_type thd_handle;
    thread_id id;
};


}
\$\endgroup\$
2
  • \$\begingroup\$ I might be missing something, but why couldn't scoped_thread be implemented in terms of std::thread? \$\endgroup\$
    – user786653
    Feb 11 '20 at 17:51
  • \$\begingroup\$ It could, but I wanted to make use of the new language features and to be more familiar with templates \$\endgroup\$
    – dev65
    Feb 12 '20 at 11:36
2
\$\begingroup\$

Overall I think the code is pretty good and your use of the more modern features of C++ looks OK.

Still there are some things that could be improved. In no particular order I offer the following suggestions:

  • The idea of a scoped_thread is good (also discussed in Effective Modern C++ item 37). Even if you want to implement your own std::thread replacement (for learning purposes or otherwise), I'd recommend splitting that part out and implementing scoped_thread in terms of it. That way you can re-use both classes in other contexts.
  • I'd probably use either the functions from process.h or the raw Win32 API (i.e. CreateThread) not mix them.
  • Speaking of Win32 API functions: Just include windows.h, don't do the declarations on your own.
  • I realize including windows.h in a header file like this is pollutes the global namespace, so I'd create wrapper functions like void native_start_thread(void (*)(void*)) etc. and place them in an accompanying C++ file.
  • In modern C++ naked calls to delete are a code smell. You should always have the pointers owned by a std::unique_ptr. Of course you have to be careful in this case to not double delete objects. What I mean is: In ThreadFunc hand ownership to a std::unique_ptr and in the scoped_thread constructor store params in a unique_ptr. Use get() when creating the thread and then release() when the thread has been constructed (and thus has taken ownership of the memory).
  • You should check the return value of WaitForSingleObject
  • The code could use some more comments. For instance I had to look up whether 0 could be a valid thread id (it can't, but I couldn't remember that was the case).
  • I can't see anything wrong with the code in this regard, but I'm not too fond of thread ownership actually being tied to the thread id.
  • Again, I don't see any immediate problems, but I'd probably make sure to null out the thread handle in the moved from thread (to at least aid in debugging). This would mean using a non-defaulted move-constructor, and then I'd probably implement it in terms of the move assignment operator (which then wouldn't/couldn't use placement new, which again I think works fine, but seems "overkill" here).
\$\endgroup\$
2
  • 1
    \$\begingroup\$ "I'd probably use either the functions from process.h or the raw Win32 API (i.e. CreateThread) not mix them" . the docs recommends to start any thread that uses c runtime library using functions from process.h in order to avoid a small memory leak when the thread exits . for the last remark : it's already done because the thread handle is a smart handle (handle in a smart pointer) so the default unique_ptr move assignment operator nulls out (invalidates) the handle once moved from to ensure one object manages the handle . thanks for your tips . \$\endgroup\$
    – dev65
    Feb 12 '20 at 18:04
  • \$\begingroup\$ @dev65: Sorry my bad on both counts. I mistakenly though the need to use _beginthread was gone, but it seems like that's not the case when linking statically to the CRT. And in the heat of battle I'd forgotten that native_handle_type was a smart pointer - I'll amend my post later to reflect this. \$\endgroup\$
    – user786653
    Feb 12 '20 at 18:25

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.