Server socket doesn't work properly - "accept is already open"

Question

I've tried to separate my server socket in a singleton. Here's the code:

ServerSocket.h

#pragma once
#include <asio.hpp>
#include <iostream>

using asio::ip::tcp;

class ServerSocket
{
public:
    ServerSocket(ServerSocket& otherSingleton) = delete;
    void operator=(const ServerSocket& copySingleton) = delete;

    tcp::acceptor* InitAcceptor();
    tcp::socket* InitSocket();
    void StartServerSocket();
    void SendData(std::string);
    std::array<char, 5000> RecieveData();

    static ServerSocket* GetInstance();
private:
    static ServerSocket* instance;

    tcp::acceptor* acceptor;
    tcp::socket* socket;
    asio::io_context io_context;

    ServerSocket() {
        acceptor = InitAcceptor();
        socket = InitSocket();
    }

    ~ServerSocket()
    {
        std::cout << "Server closed";
    }
};

ServerSocket.cpp

#include "ServerSocket.h"

tcp::acceptor* ServerSocket::InitAcceptor()
{
    try
    {
        tcp::acceptor* acceptor = new tcp::acceptor(io_context, tcp::endpoint(tcp::v4(), 27015));

        return acceptor;
    }
    catch (std::exception& e)
    {
        std::cerr << e.what() << std::endl;
    }
}

tcp::socket* ServerSocket::InitSocket()
{
    try
    {
        tcp::socket* socket = new tcp::socket(io_context);

        return socket;
    }
    catch (std::exception& e)
    {
        std::cerr << e.what() << std::endl;
    }
}

void ServerSocket::StartServerSocket()
{
    try
    {
        std::cout << "Server started";
        for (;;)
        {
            acceptor->accept(*socket);
        };
    }
    catch (std::exception& e)
    {
        std::cerr << e.what() << std::endl;
    }
}

std::array<char, 5000> ServerSocket::RecieveData()
{
    try {
        std::array<char, 5000> buf;
        asio::error_code error;

        size_t len = socket->read_some(asio::buffer(buf), error);
        buf[len] = '\0';
        return buf;
    }
    catch (std::exception& e)
    {
        std::cerr << e.what() << std::endl;
    }
}

ServerSocket* ServerSocket::instance(nullptr);

ServerSocket* ServerSocket::GetInstance()
{
    if (instance == nullptr)
    {
        instance = new ServerSocket();
    }
    return instance;
}

Server socket starts, I get:

Server started

when a client connects, I get:

accept: Already open 

and the server stops.

I think the error comes from the acceptor being in a for function. But according to the docs, it should work this way. (or at least that's how I understand - https://think-async.com/Asio/asio-1.20.0/doc/asio/tutorial/tutdaytime2.html)

I tried deleting the for loop, like this:

try
    {
        std::cout << "Server started";
        acceptor->accept(*socket);
    }

and now there is no problem. But the connection isn't kept open by the server. The client connects once, sends data, and the server stops running.

As far as I understand from the docs, if I set the acceptor in a for(;;), it should be running - but it doesn't work in my case.

So, how can I keep my socket open in my implementation? I want it to be running for more than one SendData - I want it to be able to communicate with the client as long as the client is connected.

Thanks.

//Edit:

Here's the client code:

#include <iostream>
#include <asio.hpp>
#include "../../cereal/archives/json.hpp"

using asio::ip::tcp;

int main(int argc, char* argv[])
{

    try
    {
        if (argc != 2)
        {
            std::cerr << "Usage: client <host>" << std::endl;
            return 1;
        }

        // Socket Parameters
        const unsigned port = 27015;
        auto ip_address = asio::ip::make_address_v4(argv[1]);
        auto endpoint = tcp::endpoint{ ip_address, port };

        // Creating and Connecting the Socket
        asio::io_context io_context;
        auto resolver = tcp::resolver{ io_context };
        auto endpoints = resolver.resolve(endpoint);

        auto socket = tcp::socket{ io_context };
        asio::connect(socket, endpoints);

        std::array<char, 5000> buf;
        std::cout << "Message to server: ";

        asio::error_code ignored_error;

        std::string username = "test", password = "mihai";
        std::stringstream os;
        {
            cereal::JSONOutputArchive archive_out(os);
            archive_out(
                CEREAL_NVP(username),
                CEREAL_NVP(password)
            );
        }

        asio::write(socket, asio::buffer(os.str()), ignored_error);
        return 0;
    }
    catch (std::exception& e)
    {
        std::cerr << e.what() << std::endl;
        return 1;
    }

And Communication.h which is responsible to catching the operation from the client and sending it to the server

#pragma once
#include <iostream>
#include "DBUser.h"
#include "DBPost.h"

class Communication
{
public:
    enum class Operations {
        eLogin,
        eRegister
    };
    void ExecuteOperation(Operations operation,const std::array<char, 5000>& buffer);
};

.cpp

#include "Communication.h"

void Communication::ExecuteOperation(Operations operation,const std::array<char, 5000>& buffer)
{
    DBUser* user= DBUser::getInstance();
    switch (operation)
    {
    case Communication::Operations::eLogin:
    {
        std::string username, password;
        std::stringstream is(buffer.data());
        {
            cereal::JSONInputArchive archive_in(is);
            archive_in(username,password);
        }
        try
        {
            user->LoginUser(username, password);
        }
        catch (const std::exception& e)
        {
            std::cout << e.what();
        }
        break;
    }
    case Communication::Operations::eRegister:
    {
        std::string username, password;
        std::stringstream is(buffer.data());
        {
            cereal::JSONInputArchive archive_in(is);
            archive_in(username, password);
        }
        try
        {
            user->CreateUser(username, password);
        }
        catch (const std::exception& e)
        {
            std::cout << e.what();
        }
        break;
    }
    }
}

Main

#include <iostream>
#include <pqxx/pqxx>
#include "DBLink.h"
#include "DBUser.h"
#include "DBPost.h"
#include "../Logging/Logging.h"
#include <iostream>
#include <string>
#include <asio.hpp>
#include "ServerSocket.h"
#include "Communication.h"

int main()
{
    ServerSocket* test = ServerSocket::GetInstance();
    test->StartServerSocket();

    std::array<char, 5000> buf = test->RecieveData();
    Communication communicationInterface;
    communicationInterface.ExecuteOperation(Communication::Operations::eRegister, buf);
system("pause");
}

Answer 1

There's a lot of antipattern going on.

1. Overuse of pointers.

2. Overuse of new (without any delete, a guaranteed leak)

3. The destructor claims that "Server closed" but it doesn't actually do a single thing to achieve that.

4. Two-step initialization (InitXXXX functions). Firstly, you should obviously favor initializer lists

   ServerSocket()
    : acceptor_(InitAcceptor()), socket_(InitSocket())
   { }
   

   And you need to makeInitAcceptor/InitSocket private to the implementation.

5. I'll forget the Singleton which is anti-pattern 99% of the time, but I guess that's almost debatable.

In your StartServerSocket you have a loop that reuses the same socket all the time. Of course, it will already be connected. You need separate socket instances:

    for (;;) {
        acceptor_->accept(*socket_);
    };

Simplify/Fix

#include <boost/asio.hpp>
#include <iostream>

namespace asio = boost::asio;
using asio::ip::tcp;

struct Listener {
    void Start()
    {
        std::cout << "Server started";
        for (;;) {
            auto socket = acceptor_.accept();
            std::cout << "Accepted connection from " << socket.remote_endpoint()
                      << std::endl;
        };
    }

    static Listener& GetInstance() {
        static Listener s_instance{27015}; // or use weak_ptr for finite lifetime
        return s_instance;
    }

  private:
    asio::io_context ioc_; // order of declaration is order of init!
    tcp::acceptor    acceptor_;

    Listener(uint16_t port) : acceptor_{ioc_, tcp::endpoint{tcp::v4(), port}} {}
};

int main() {
    try {
        Listener::GetInstance().Start();
    } catch (std::exception const& e) {
        std::cerr << e.what() << std::endl;
    }
}

Now you could hand the socket instances to a thread. I concur with the other commenters that thread-per-request is likely also an anti-pattern, and you should consider using async IO with Asio (hence the name).

Live Demo