samedi 15 octobre 2022

Is this an error in the Effective Modern C++ book?

Item 37 in the book illustrates an implementation of ThreadRAII, that either joins a thread or detaches it when it is destroyed. By declaring the destructor the compiler doesn’t generate the move operations, but in the book the author says that there’s no reason why they should not be movable and says that compilers would generate the right move operations and advises us to use the ‘= default’ implementation.

#include <thread>

class ThreadRAII
{
public:
    enum class DtorAction { join, detach };

    ThreadRAII(std::thread&& t, DtorAction a)
        : action(a)
        , t(std:: move(t))
    {}

    ~ThreadRAII()
    {
        if(t.joinable())
        {
            if(action == DtorAction::join)
                t.join();
            else
                t.detach();
        }
    }

    ThreadRAII(ThreadRAII&&) = default;

    ThreadRAII& operator=(ThreadRAII&&) = default;

    std::thread& get() { return t; }

private:
    DtorAction action;
    std::thread t;
};

int main()
{
    ThreadRAII t{std::thread{[]{}}, ThreadRAII::DtorAction::join};

    t = ThreadRAII{std::thread{[]{}}, ThreadRAII::DtorAction::detach};

    return 0;
}

But in the example above, std::terminate is called.

I think that the default move constructor should be ok, but not the move assignment, because the move assignment has to release the current resource before acquiring the new one. Otherwise, the assignment is going to destroy a thread that is joinable, which causes program termination.

I didn't see this issue in the errata list for the book. Is the book really wrong by saying the default move assignment operator should be fine? I would like to be sure and have other people look at it in order to contact the author.

This is what I think it should have been:

#include <thread>

class ThreadRAII
{
public:
    enum class DtorAction { join, detach };

    ThreadRAII(std::thread&& t, DtorAction a)
        : action(a)
        , t(std:: move(t))
    {}

    ~ThreadRAII()
    {
        release();
    }

    ThreadRAII(ThreadRAII&&) = default;

    ThreadRAII& operator=(ThreadRAII&& rhs)
    {
        release();
        action = rhs.action; 
        t = std::move(rhs.t);
        return *this;
    }

    std::thread& get() { return t; }

    void release()
    {
        if(t.joinable())
        {
            if(action == DtorAction::join)
                t.join();
            else
                t.detach();
        }
    }

private:
    DtorAction action;
    std::thread t;
};

int main()
{
    ThreadRAII t{std::thread{[]{}}, ThreadRAII::DtorAction::join};

    t = ThreadRAII{std::thread{[]{}}, ThreadRAII::DtorAction::detach};

    return 0;
}

Aucun commentaire:

Enregistrer un commentaire