Passing RAII socket class to thread

Question

I am making a HTTP server and I want to pass a socket client class to thread to handle the request. The problem is that the destructor gets called right after starting the thread so the socket gets closed. The simplified version of code looks like this.

while (true) {
    TcpSocket client = m_socket.accept();
    std::thread clientThread([this, &client] { this->handleClient(client); });
    clientThread.detach();
}

I tried making a function that accepts a connection and passes it to a thread with a given function but I can't get it to work. I am using a tuple here because you can't pass a parameter pack to lambda in c++17.

template<typename Function, typename ...Args>
std::thread TcpListener::acceptAndPassToThread(Function&& function, Args&&... args)
{
    int socketDescriptor = accept(m_socketDescriptor, nullptr, nullptr);

    std::tuple arguments = std::make_tuple(std::forward<Args>(args)...);

    std::thread clientThread([socketDescriptor, function, arguments] {
        TcpSocket client = TcpSocket(socketDescriptor);

        auto callArguments = std::tuple_cat(std::make_tuple(client), arguments);
        std::apply(function, callArguments);

    });

    return clientThread;
}

I could do something like this but I would prefer not to use socket descriptors outside the class.

while (true)
{
    int clientDescriptor = m_socket.acceptDescriptor();

    std::thread clientThread([this, clientDescriptor] {
        TcpSocket client = TcpSocket::fromDescriptor(clientDescriptor);
        this->handleClient(client);
    });

    clientThread.detach();
}

Also what would be the proper way to call the TcpListener::acceptAndPassToThread function with a class member function.

Galik · Accepted Answer · 2021-07-05T11:07:18.997

Your RAII class TcpSocket should really be move only because it is logically wrong to make copies. Of course you could implement it like a std::shared_ptr but then the value semantics are a bit misleading. It would then probably be better to call it something that reflects its shared nature.

To make a class move only you delete the copy constructor and the copy assignment operator and provide move versions of both. Something like this:

class TcpSocket
{
public:
    // ...

    ~TcpSocket() { this->close(); }

    TcpSocket(int descriptor = -1): descriptor(descriptor) {}
  
    // ban copying  
    TcpSocket(TcpSocket const&) = delete;
    TcpSocket& operator=(TcpSocket const&) = delete;

    // implement move so the moved from object will
    // not close this descriptor    
    TcpSocket(TcpSocket&& other): TcpSocket()
    {
        std::swap(descriptor, other.descriptor);
    }

    // same with move assignment
    // this closes any current descriptor but you may
    // want to throw an exception if this descriptor
    // is currently in use.    
    TcpSocket& operator=(TcpSocket&& other)
    {
        if(&other != this)
            this->close(); // or throw?

        std::swap(descriptor, other.descriptor);

        return *this;
    }

private:
    void close()
    {
        if(descriptor != -1)
            ::close(descriptor);
        descriptor = -1;
    }

    int descriptor = -1;
};

Then move the socket into the thread:

while (true) {
    TcpSocket client = m_socket.accept();
    std::thread clientThread([this, client = std::move(client)] mutable {
        this->handleClient(std::move(client));
    });

    clientThread.detach(); // looks dangerout?
}

Thank you for your answer. I tried implementing it but on the second move I get an error "void HttpServer::handleClient(TcpSocket &)': cannot convert argument 1 from 'const TcpSocket' to 'TcpSocket &&". I don't know why the object is const. Also what does "TcpSocket const&" mean. — Wojak2121, Jul 05 '21 at 00:15
@Wojak2121 Soz, the code is untested. It's probably that the llambda needs to be *mutable* (llambdas are *const* by default). I modified the answer to demonstrate. BTW `TcpSocket const&`is the same as `const TcpSocket&` — Galik, Jul 05 '21 at 01:24

score 0 · Answer 2 · answered Jul 04 '21 at 22:32

If you have control over the TcpSocket class, I would recommend you to implement the pimpl idiom by wrappping the private implementation member inside a shared_ptr (or unique_ptr if you use it right).

To fix your immediate issue, you could just use std::shared_ptr<TcpSocket> instead of the raw class, like this:

while (true) {
    std::shared_ptr<TcpSocket> client = m_socket.accept();
    std::thread clientThread([this, client] { this->handleClient(client); });
    clientThread.detach();
}

void handleClient(std::shared_ptr<TcpSocket> client) {
    // ...
}

You will have to change the function m_socket.accept() in order to return a shared_ptr.

There is 0 reason to use dynamic allocation for TcpSocket, and even less so to use shared_ptr. The simple fact you need to tightly couple your interface to how you use it on call site should already be a major red flag. — spectras, Jul 04 '21 at 23:38

Passing RAII socket class to thread

2 Answers2