0

I created the main cpp file and three classes to create an asynchronous server. Server, Service, and Acceptor respectively. However, they caused errors in the build process, even though there were no errors in the visual studio 2019 environment. I tried to fix the error, but most of the errors occurred in other files, so I couldn't even think of it myself.

main

#include "Server.h"
#include <iostream>
#include <boost/asio.hpp>
#include <thread>
#include <atomic>
#include <memory>
#define DEFAULT_THREAD_SIZE 2;
using namespace boost;
int main()
{
 unsigned short port_num;
    std::cin >> port_num;
    try {
        Server srv;
        unsigned int threads = std::thread::hardware_concurrency() * 2;
        if (threads == 0) {
            threads = DEFAULT_THREAD_SIZE;
        }
        std::cout << "\nPort - " << port_num << "\nServer start\n";
        srv.Start(port_num, threads);
        while (1) {
            std::cin >> srv;
        }
    }
    catch (system::system_error& e) {
        std::cout << "\nError code: " << e.code() << "\nError Message\n" << e.what();
    }
    return 0;
}

This includes Server.h, which defines Server class.

#include "Acceptor.h"
#include <boost/asio.hpp>
#include <thread>
#include <atomic>
#include <memory>
using namespace boost;
class Server
{
public:
    Server();
    void Start(unsigned short port_num, unsigned int threads);
    void Stop();
    int Command(std::string& str);
private:
    asio::io_service mios;
    std::unique_ptr<asio::io_service::work> mWork;
    std::unique_ptr<Acceptor> mAcceptor;
    std::vector <std::unique_ptr<std::thread>> mThreads;
};

std::istream& operator>>(std::istream& is, Server& srv);

Here's the implementation, Server.cpp.

#include "Server.h"
Server::Server() {
    mWork.reset(new asio::io_service::work(mios));
}
void Server::Start(unsigned short port_num, unsigned int threads) {
    assert(thread > 0);
    mAcceptor.reset(new Acceptor(mios, port_num));
    mAcceptor->Start();
    for (int i = 0; i < threads; i++) {
        std::unique_ptr<std::thread> th(new std::thread([this]() {mios.run(); }));
        mThreads.push_back(std::move(th));
    }
}
void Server::Stop() {
    mAcceptor->Stop();
    mios.stop();

    for (auto& th : mThreads) {
        th->join();
    }
}
int Server::Command(std::string& str) {
    return 0;
}
std::istream& operator>>(std::istream& is, Server& srv) {
    std::string str;
    is >> str;
    srv.Command(str);
    return is;
}

This is Acceptor class.

#include "Service.h"
#include <boost/asio.hpp>
#include <thread>
#include <atomic>
#include <memory>
using namespace boost;
class Acceptor
{
public:
    Acceptor(asio::io_service& ios, unsigned short port_num);
    void Start();
    void Stop();
private:
    std::shared_ptr<asio::io_service> mios;
    std::shared_ptr<asio::ip::tcp::acceptor> mAcceptor;
    std::atomic<bool> mIsStopped;
    void InitAccept();
    void OnAccept(const system::error_code ec, std::shared_ptr<asio::ip::tcp::socket> sock);
};
#include "Acceptor.h"
Acceptor::Acceptor(asio::io_service& ios, unsigned short port_num) {
    mios = std::make_shared<asio::io_service>(ios);
    mAcceptor = std::make_shared<asio::ip::tcp::acceptor>(mios, asio::ip::tcp::endpoint(asio::ip::address_v4::any(), port_num));
    mIsStopped = false;
}
void Acceptor::Start() {
    mAcceptor->listen();
    InitAccept();
}
void Acceptor::Stop() {
    mIsStopped.store(true);
}

void Acceptor::InitAccept() {
    std::shared_ptr<asio::ip::tcp::socket> sock(new asio::ip::tcp::socket(mios));
    mAcceptor->async_accept(*sock, [this, sock](const system::error_code& error) {OnAccept(error, sock);});
}
void Acceptor::OnAccept(const system::error_code ec, std::shared_ptr<asio::ip::tcp::socket> sock) {
    if (ec.value() == 0 || ER) {
        (new Service(sock))->StartHandling();
    }
    else{
        std::cout << "Error code:" << ec.value() << "error " << "Error message: " << ec.message() << "\n";
    }
    if (!mIsStopped.load()) {
        InitAccept();
    }
    else {
        mAcceptor->close();
    }
}

Service class

#define ER true
#include <iostream>
#include <boost/asio.hpp>
#include <thread>
#include <atomic>
#include <memory>
using namespace boost;
class Service
{
public:
    Service(std::shared_ptr<asio::ip::tcp::socket> sock);
    void StartHandling();
private:
    void OnRequestReceived(const boost::system::error_code& ec, std::size_t bytes_transferred);
    std::string mReponse;
    std::shared_ptr<asio::ip::tcp::socket> mSock;
    asio::streambuf mRequest;
    void OnReponseSent(const system::error_code& ec, std::size_t bytes_transferred);

    void OnFinish();
    std::string ProcessRequest(asio::streambuf& request);
};
#include "Service.h"
Service::Service(std::shared_ptr<asio::ip::tcp::socket> sock){
    mSock = sock;
}
void Service::StartHandling() {
    asio::async_read_until(mSock, mRequest, '\n', [this](const system::error_code ec, std::size_t bytes_transferred) {OnRequestReceived(ec, bytes_transferred); });
}

void Service::OnRequestReceived(const system::error_code& ec, std::size_t bytes_transferred) {
    if (ec.value() != 0 || ER) {
        std::cout << "Error code:" << ec.value() << "Error message: " << ec.message() << "\n";
        OnFinish();
        return;
    }
    mReponse = ProcessRequest(mRequest);
    asio::async_write(mSock, asio::buffer(mReponse), [this](const system::error_code& ec, std::size_t bytes_transferred) {OnReponseSent(ec, bytes_transferred); });
}

void Service::OnReponseSent(const system::error_code& ec, std::size_t bytes_transferred) {
    if (ec.value() != 0 || ER) {
        std::cout << "Error code:" << ec.value() << "Error message: " << ec.message() << "\n";
    }
    OnFinish();
}

void Service::OnFinish() {
    delete this;
}
std::string Service::ProcessRequest(asio::streambuf& request) {
    std::string reponse;
    std::istream input(&request);
    std::getline(input, reponse);
    assert(reponse.back() == '\n');
    return reponse;
}

I have no idea what to do. I wanted to do it myself, but I couldn't even debug because I couldn't figure out where the problem was going and it wasn't built.

Jesper Juhl
  • 1
  • 3
  • 38
  • 63
xnote
  • 9

1 Answers1

4

It simply doesn't compile. I genuinely wonder how people can come up with /so much/ code before noticing that stuff doesn't compile.

Rule #1: Baby Steps (this goes for the professionals just as much, only they have it internalized).

  • You're doing stuff like:

    mios = std::make_shared<asio::io_service>(ios);
    

    This requires io_service to be copyable (which it isn't). You would probably make mios a reference:

    asio::io_service& mios;
    
  • There seems to be a lot of "superstitious" use of shared_ptr all around.

  • The fact that

    assert(thread > 0);
    

    misspelled threads indicates that you may have been building Release-only builds.

  • Read the compiler messages:

    void Service::StartHandling() {
        asio::async_read_until(mSock, mRequest, '\n', [this](const system::error_code ec, std::size_t bytes_transferred) {OnRequestReceived(ec, bytes_transferred); });
    }
    

    This triggers the error:

    /home/sehe/custom/boost_1_73_0/boost/asio/impl/read_until.hpp|959 col 53| error: no type named ‘executor_type’ in ‘class std::shared_ptr<boost::asio::basic_stream_socket<boost::asio::ip::tcp> >’
    

    Obviously you meant *mSock. Same later:

    asio::async_write(*mSock, asio::buffer(mReponse), [this](const system::error_code& ec, std::size_t bytes_transferred) {OnReponseSent(ec, bytes_transferred); });
    

    A pointer is not the object it points to - not even smart pointers. The point [sic] of smart pointers is not to make C++ equal to (say) Java - you should use Java if you wanted that.

With these it compiles: Live ON Wandbox

More Review

  • top-level const makes no difference in value arguments

  • Don't use new or delete:

     mWork.reset(new asio::io_service::work(mios));
    

    use make_unique instead

     mWork = std::make_unique<asio::io_service::work>(mios);
     // ...
     mAcceptor = std::make_unique<Acceptor>(mios, port_num);
    
  • Use header guards (or #pragma once)

  • Do not use namespace using-directives; use using-declarations instead

  • Especially don't use namespace using-directives in header files (you make it impossible for your users to prevent/fix name collisions, which may lead to compile error or silent change of behaviour)

  • Use constructor initializer lists (and move-semantics):

     Service::Service(std::shared_ptr<asio::ip::tcp::socket> sock){
         mSock = sock;
     }
    

    Becomes

     Service::Service(std::shared_ptr<asio::ip::tcp::socket> sock) 
         : mSock(std::move(sock))
     { }
    
  • Here:

     (new Service(std::move(sock)))->StartHandling();
    

    Don't use new, don't superstitious-use shared pointer, and, ironically, in the case of Service consider using enable_shared_from_this so you do use shared_ptr instead of the delete this; anti-pattern.

  • Initialize your primitive class members1

     std::atomic<bool> mIsStopped{};
    

    Without, it will have indeterminate value, which usually leads to UB when used

  • Don't ignore errors:

     if (ec.value() == 0 || ER) {
         (new Service(std::move(sock)))->StartHandling();
     }
    

    Instead, report / log. Also, detect errors portably:

     if (!ec) {
    

    Or

     if (!ec.failed()) {
    
  • generally, handle errors (cin >> port_num e.g.),

  • catch by const&

Intermediate result (still compiles): Live on Wandbox

BONUS

  • Simplify, use asio::thread_pool, uniform initalization

  • USE bytes_transferred! read_until does not guarantee it stops on the delimiter, because that's not how TCP works. Trailing data can be present in the buffer. This means that in DEBUG builds this assert would sometimes fail:

     assert(request.back() == '\n');
    

    Actually the code read response.back() which is guaranteed to fail because getline doesn't include it ¯\(ツ)

    You might use boost::iostreams::restrict or instead asio::dynamic_buffer() on a std::string and pass a string_view into the handler (ProcessRequest):

     mReponse = ProcessRequest(std::string_view(mRequest).substr(0, bytes_transferred));
    

    And later

     #include <boost/iostreams/device/array.hpp>
     #include <boost/iostreams/stream_buffer.hpp>
    
     std::string Service::ProcessRequest(std::string_view request) {
         assert(request.back() == '\n');
         boost::iostreams::stream_buffer<boost::iostreams::array_source> buf(
             request.data(), request.size());
    
         std::istream input(&buf);
         std::string reponse;
         std::getline(input, reponse);
         return reponse;
     }
    
  • Get rid of all the redundant shared pointers. If Acceptor is already dynamically allocated managed by a shared-pointer, there is really no need to also make it own its tcp::acceptor instance by shared_ptr. In general all the members could just be by value in your code. As long as the surrounding object stays around (as you do with Service) the members are guaranteed to stay alive as well.

  • mIsStopped can be eliminated by simply cancel()-ing the acceptor instead. To get thread-safety, simply post to the relevant executor.

  • If you wanted the server to actually exit when the stop command is executed, you need to make the while(true) loop have a stop condition, e.g.

     int Server::Command(std::string const& cmd) {
         std::cout << "Command: " << std::quoted(cmd) << "\n";
         if (cmd == "quit") {
             Stop();
             return 1;
         }
         std::cerr << "Unknown command (\"quit\" to exit)" << std::endl;
         return 0;
     }
    
     std::istream& operator>>(std::istream& is, Server& srv) {
         std::string str;
         is >> str;
         if (srv.Command(str)) {
             is.setstate(std::ios::badbit);
         }
         return is;
     }
    

    And in main:

         while (std::cin >> srv) { }
    

FULL DEMO:

Live On Wandbox

  • File Acceptor.h

     #ifndef _HOME_SEHE_PROJECTS_STACKOVERFLOW_ASIO_ACCEPTOR_H
     #define _HOME_SEHE_PROJECTS_STACKOVERFLOW_ASIO_ACCEPTOR_H
    
     #include "Service.h"
     class Acceptor {
       public:
         template <typename Executor>
         Acceptor(Executor ex, unsigned short port_num) : mAcceptor(make_strand(ex), {{}, port_num}) {}
         void Start();
         void Stop();
    
       private:
         tcp::acceptor mAcceptor;
         void InitAccept();
         void OnAccept(error_code ec, tcp::socket&& sock);
     };
    
     #endif
    
  • File Common.h

     #pragma once
     #include <boost/asio.hpp>
     #include <memory>
     #include <thread>
     #include <atomic>
    
     namespace asio = boost::asio;
     using boost::system::error_code;
     using asio::ip::tcp;
    
  • File Server.h

     #ifndef _HOME_SEHE_PROJECTS_STACKOVERFLOW_ASIO_SERVER_H
     #define _HOME_SEHE_PROJECTS_STACKOVERFLOW_ASIO_SERVER_H
    
     #include "Acceptor.h"
     class Server {
       public:
         explicit Server(unsigned short port_num);
         void Start();
         void Stop();
         int Command(std::string const& str);
    
       private:
         asio::thread_pool mio;
         Acceptor mAcceptor;
     };
    
     std::istream& operator>>(std::istream& is, Server& srv);
    
     #endif
    
  • File Service.h

     #ifndef _HOME_SEHE_PROJECTS_STACKOVERFLOW_ASIO_SERVICE_H
     #define _HOME_SEHE_PROJECTS_STACKOVERFLOW_ASIO_SERVICE_H
    
     #include "Common.h"
     #include <iostream>
     class Service : public std::enable_shared_from_this<Service> {
       public:
         explicit Service(tcp::socket&& sock);
         void StartHandling();
    
       private:
         void OnRequestReceived(error_code ec, std::size_t bytes_transferred);
         std::string mRequest, mReponse;
         tcp::socket mSock;
         void OnReponseSent(error_code ec, size_t bytes_transferred);
    
         std::string ProcessRequest(std::string_view request);
     };
    
     #endif
    
  • File Acceptor.cpp

     #include "Acceptor.h"
     #include <utility>
    
     void Acceptor::Start() {
         mAcceptor.listen();
         InitAccept();
     }
    
     void Acceptor::Stop() {
         // be thread safe
         post(mAcceptor.get_executor(), [this] { mAcceptor.cancel(); });
     }
    
     void Acceptor::InitAccept() {
         mAcceptor.async_accept(
             make_strand(mAcceptor.get_executor()),
             [this](error_code error, tcp::socket&& sock) { OnAccept(error, std::move(sock)); });
     }
    
     void Acceptor::OnAccept(error_code ec, tcp::socket&& sock) {
         if (!ec.failed()) {
             std::make_shared<Service>(std::move(sock))->StartHandling();
             InitAccept();
         } else {
             std::cout << "OnAccept: " << ec.message() << "\n";
         }
     }
    
  • File main.cpp

     #include "Server.h"
     #include <iostream>
    
     int main() {
         if (uint16_t port_num; std::cin >> port_num) {
             try {
                 Server srv(port_num);
                 std::cout << "Port - " << port_num << "\nServer start\n";
                 srv.Start();
    
                 while (std::cin >> srv) { }
             } catch (boost::system::system_error const& e) {
                 std::cout << "Error " << e.code().message() << "\n";
             }
         } else {
             std::cerr << "Invalid input (port number required)\n";
         }
     }
    
  • File Server.cpp

     #include "Server.h"
     #include <iomanip>
    
     Server::Server(unsigned short port_num)
         : mAcceptor(make_strand(mio), port_num) {}
    
     void Server::Start() { mAcceptor.Start(); }
     void Server::Stop() { mAcceptor.Stop(); }
    
     int Server::Command(std::string const& cmd) {
         std::cout << "Command: " << std::quoted(cmd) << "\n";
         if (cmd == "quit") {
             Stop();
             return 1;
         }
         std::cerr << "Unknown command (\"quit\" to exit)" << std::endl;
         return 0;
     }
    
     std::istream& operator>>(std::istream& is, Server& srv) {
         std::string str;
         is >> str;
         if (srv.Command(str)) {
             is.setstate(std::ios::badbit);
         }
         return is;
     }
    
  • File Service.cpp

     #include "Service.h"
     #include <utility>
     #include <iomanip>
    
     Service::Service(tcp::socket&& sock)
             : mSock(std::move(sock)) {}
    
     void Service::StartHandling() {
         asio::async_read_until(
             mSock, asio::dynamic_buffer(mRequest), '\n',
             [this, self = shared_from_this()](error_code ec, std::size_t bytes_transferred) {
                 OnRequestReceived(ec, bytes_transferred);
             });
     }
    
     void Service::OnRequestReceived(error_code ec, std::size_t bytes_transferred) {
         if (ec) {
             std::cout << "OnRequestReceived: " << ec.message() << "\n";
             return;
         }
    
         std::string_view view = mRequest;
         mReponse = ProcessRequest(view.substr(0, bytes_transferred));
    
         asio::async_write(
                 mSock, asio::buffer(mReponse),
                 [this, self = shared_from_this()](error_code ec, std::size_t bytes_transferred) {
                     OnReponseSent(ec, bytes_transferred);
                 });
     }
    
     void Service::OnReponseSent(error_code ec, std::size_t /*bytes_transferred*/) {
         if (ec) {
             std::cout << "OnReponseSent: " << ec.message() << "\n";
         }
     }
    
     #include <boost/iostreams/device/array.hpp>
     #include <boost/iostreams/stream_buffer.hpp>
    
     std::string Service::ProcessRequest(std::string_view request) {
         //std::cerr << "TRACE: " << std::quoted(request) << "\n";
         assert(request.back() == '\n');
         boost::iostreams::stream_buffer<boost::iostreams::array_source> buf(
             request.data(), request.size());
    
         std::istream input(&buf);
         std::string reponse;
         std::getline(input, reponse);
         return reponse + '\n';
     }
    

E.g. when running with 2323 and later quit command:

# (echo 2323; sleep 30; echo quit) | ./sotest
Port - 2323
Server start
Command: "quit"
OnAccept: Operation canceled

It does correctly accept multiple connections:

# for a in {1..10}; do printf "Message with random data $RANDOM\n" | nc localhost 2323; done
Message with random data 8002
Message with random data 28046
Message with random data 17943
Message with random data 17845
Message with random data 10832
Message with random data 20049
Message with random data 27593
Message with random data 18979
Message with random data 2773
Message with random data 31159
sehe
  • 328,274
  • 43
  • 416
  • 565
  • Added a deeper review with many simplifications. Tested and confirmed UBSAN/ASAN clean: https://wandbox.org/permlink/vq3oFBgqB9CpbQjD – sehe Aug 08 '20 at 15:41
  • I used strands - your approach using `atomic` looked valid to me because you only used it on a "logical strand", but see https://stackoverflow.com/questions/12794107/why-do-i-need-strand-per-connection-when-using-boostasio/12801042#12801042 – sehe Aug 08 '20 at 15:45
  • For convenience, all the files in a gist: https://gist.github.com/sehe/e26c33ce9c845966a83850b2dea3f207 – sehe Aug 08 '20 at 16:08