4
\$\begingroup\$

This is my first time trying to do anything proper in Boost::Asio, so I would love if someone could look at it and tell me if it makes sense. It's based on examples from documentation and bit of googling around, but I want to know if I've put it correctly together.

I know this code doesn't do anything useful. I want to know if foundations are correct before putting more stuff in.

main.cpp

#include <iostream>

#include "server.h"

using namespace std;
using namespace wot;

int main(int, char **) {
    try {
        ios_base::sync_with_stdio(false);

        server s("0.0.0.0", 0xc0fe);
        s.run();

        return 0;
    } catch (exception & e) {
        cerr << "Unhandled exception: " << e.what() << endl;
        return 1;
    }
}

server.h

#ifndef SERVER_H_SFH9W7WB
#define SERVER_H_SFH9W7WB

#include <boost/asio.hpp>

#include "connection.h"
#include "connection_manager.h"

namespace wot {

    class server {
    public:
        server(const server &) =delete;
        server & operator=(const server &) =delete;

        explicit server(const std::string & addr, const size_t port);

        void run();
    private:
        void start_accept();
        void handle_accept(connection::ptr, const boost::system::error_code &);
        void start_await_stop();

        boost::asio::io_service ios_;
        boost::asio::signal_set signals_;
        boost::asio::ip::tcp::acceptor acceptor_;

        connection_manager connection_manager_;
    };

}

#endif /* end of include guard: SERVER_H_SFH9W7WB */

server.cpp

#include "server.h"

#include <iostream>

namespace asio = boost::asio;
namespace ip = boost::asio::ip;

using namespace wot;
using namespace std;

server::server(const std::string & addr, const size_t port)
    : ios_(),
      signals_(ios_),
      acceptor_(ios_)
{
    signals_.add(SIGINT);
    signals_.add(SIGTERM);
#if defined(SIGQUIT)
    signals_.add(SIGQUIT);
#endif

    start_await_stop();

    ip::tcp::endpoint endpoint(ip::address::from_string(addr), port);

    acceptor_.open(endpoint.protocol());
    acceptor_.set_option(ip::tcp::acceptor::reuse_address(true));
    acceptor_.bind(endpoint);
    acceptor_.listen();

    start_accept();
}

void server::run() {
    ios_.run();
}

void server::start_accept() {
    auto new_con = connection::create(
        acceptor_.get_io_service(),
        connection_manager_
    );
    acceptor_.async_accept(
        new_con->socket(),
        bind(&server::handle_accept, this, new_con, placeholders::_1)
    );
}

void server::handle_accept(
    connection::ptr new_con,
    const boost::system::error_code & e
) {
    if (!acceptor_.is_open()) {
        return;
    }
    if (!e) {
        connection_manager_.start(new_con);
    }
    start_accept();
}

void server::start_await_stop() {
    signals_.async_wait(
        [this](boost::system::error_code, int) {
            acceptor_.close();
            connection_manager_.stop_all();
        }
    );
}

connection_manager.h

#ifndef CONNECTION_MANAGER_H_SFH9W7WB
#define CONNECTION_MANAGER_H_SFH9W7WB

#include <set>

#include "connection.h"

namespace wot {

    class connection_manager {
    public:
        connection_manager(const connection_manager &) =delete;
        connection_manager & operator=(const connection_manager &) = delete;

        connection_manager();

        void start(connection::ptr);
        void stop(connection::ptr);
        void stop_all();
    private:
        std::set<connection::ptr> connections_;
    };

}

#endif /* end of include guard: CONNECTION_MANAGER_H_SFH9W7WB */

connection_manager.cpp

#include "connection_manager.h"

using namespace wot;
using namespace std;

connection_manager::connection_manager() {}

void connection_manager::start(connection::ptr c) {
    connections_.insert(c);
    c->start();
}

void connection_manager::stop(connection::ptr c) {
    connections_.erase(c);
    c->stop();
}

void connection_manager::stop_all() {
    for (auto c : connections_) {
        c->stop();
    }
    connections_.clear();
}

connection.h

#ifndef CONNECTION_H_KUDEQ8CR
#define CONNECTION_H_KUDEQ8CR

#include <boost/asio.hpp>

namespace wot {

    class connection_manager;

    class connection : public std::enable_shared_from_this<connection> {
    public:
        using ptr = std::shared_ptr<connection>;

        connection(const connection &) =delete;
        connection & operator=(const connection &) =delete;

        static ptr create(
            boost::asio::io_service &,
            connection_manager &
        );

        void start();
        void stop();
        boost::asio::ip::tcp::socket & socket();
    private:
        explicit connection(
            boost::asio::io_service &,
            connection_manager &
        );

        void start_read();
        void handle_read(const boost::system::error_code &, const size_t);
        void start_write();
        void handle_write(const boost::system::error_code &, const size_t);
        void handle_timeout(const boost::system::error_code &);

        boost::asio::ip::tcp::socket socket_;
        std::array<char, 8192> buffer_;

        connection_manager & connection_manager_;
        boost::asio::deadline_timer timeout_timer_;

        boost::asio::ip::tcp::endpoint remote_endpoint_;
    };

}

#endif /* end of include guard: CONNECTION_H_KUDEQ8CR */

connection.cpp

#include "connection.h"

#include "log.h"
#include "connection_manager.h"

namespace asio = boost::asio;
namespace ip = boost::asio::ip;

using namespace wot;
using namespace std;

connection::ptr connection::create(
    asio::io_service & ios,
    connection_manager & manager
) {
    return ptr(new connection(ios, manager));
}

connection::connection(
    asio::io_service & ios,
    connection_manager & manager
) :
    socket_(ios),
    connection_manager_(manager),
    timeout_timer_(ios)
{}

void connection::start() {
    remote_endpoint_ = socket_.remote_endpoint();

    log_out(remote_endpoint_, "Connection opened by peer.");

    asio::async_write(
        socket_,
        asio::buffer("Hi\n.\n"),
        bind(
            &connection::handle_write,
            shared_from_this(),
            placeholders::_1,
            placeholders::_2
        )
    );

    start_read();
}

void connection::stop() {
    boost::system::error_code ec;
    socket_.shutdown(ip::tcp::socket::shutdown_both, ec);
    if (ec) {
        log_err(
            remote_endpoint_,
            string("Error while shutting down socket: ") + ec.message()
        );
    }
    socket_.close();
    log_out(remote_endpoint_, "Socket closed.");
}

ip::tcp::socket & connection::socket() {
    return socket_;
}

void connection::start_read() {
    timeout_timer_.expires_from_now(boost::posix_time::seconds(5));
    timeout_timer_.async_wait(bind(
        &connection::handle_timeout,
        shared_from_this(),
        placeholders::_1
    ));

    socket_.async_read_some(
        asio::buffer(buffer_),
        bind(
            &connection::handle_read,
            shared_from_this(),
            placeholders::_1,
            placeholders::_2
        )
    );
}

void connection::handle_read(
    const boost::system::error_code & ec,
    const size_t bytes
) {
    if (!ec) {

        start_read();
    } else if (asio::error::eof == ec) {
        log_out(remote_endpoint_, string("Connection closed by peer."));
    } else if (asio::error::operation_aborted != ec) {
        log_out(remote_endpoint_, string("Error: ") + ec.message());
        connection_manager_.stop(shared_from_this());
    }
}

void connection::handle_write(const boost::system::error_code &, size_t) {}

void connection::handle_timeout(const boost::system::error_code & ec) {
    if (!ec) {
        log_out(remote_endpoint_, "Connection timeout.");
        connection_manager_.stop(shared_from_this());
    }
}

/*
void tcp_connection::handle_read(const boost::system::error_code & e, size_t bytes) {
    if (!e) {
        cout << "Bytes: " << bytes << endl;
        cout << "Received: " << string(begin(buffer_), begin(buffer_) + bytes) << endl;
        // append data to received stuff
        std::copy(begin(buffer_), begin(buffer_) + bytes, std::back_inserter(data_));
        auto terminator = std::search(
            begin(data_),
            end(data_),
            msg_terminator,
            msg_terminator + msg_terminator_len
        );
        if (terminator != end(data_)) {
            string msg = string(data_.begin(), terminator + msg_terminator_len);
            log_out(string("Received message: ") + msg);
            data_.erase(data_.begin(), terminator + msg_terminator_len);
        } else {
            cout << "WMessage: \"" << std::string(data_.begin(), terminator) << "\"" << endl;
        }
        cout << socket_.remote_endpoint().address().to_string() << ": Read done." << endl;
        cout << "Data: \"" << std::string(data_.data()) << "\"" << endl;
        start_read();
    } else if (e == boost::asio::error::eof) {
        log_out("Connection closed cleanly by peer.");
    } else {
        cerr << "Error: " << e.message() << endl;
        cerr << "Bytes: " << bytes << endl;
        cerr << static_cast<int>(buffer_[0]) << " " << static_cast<int>(buffer_[1]) << endl;
    }
}
}
*/

log.h

#ifndef LOG_H
#define LOG_H

#include <string>

#include <boost/asio.hpp>

namespace wot {

    void log_out(const std::string &);
    void log_out(const boost::asio::ip::tcp::endpoint &, const std::string &);

    void log_err(const std::string &);
    void log_err(const boost::asio::ip::tcp::endpoint &, const std::string &);

}

#endif

log.cpp

#include "log.h"

#include <ctime>
#include <iostream>

using namespace wot;
using namespace std;

namespace {
    void log(ostream & os, const string & msg) {
        time_t now;
        time(&now);
        char buf[sizeof "2011-10-08 07:07:09"];
        strftime(buf, sizeof buf, "%F %T", gmtime(&now));

        os << buf << ": " << msg << endl;
    }
}

void wot::log_out(const string & msg) {
    log(cout, msg);
}

void wot::log_out(
    const boost::asio::ip::tcp::endpoint & endpoint,
    const string & msg
) {
    log_out(endpoint.address().to_string() + ": " + msg);
}

void wot::log_err(const string & msg) {
    log(cerr, msg);
}

void wot::log_err(
    const boost::asio::ip::tcp::endpoint & endpoint,
    const string & msg
) {
    log_err(endpoint.address().to_string() + ": " + msg);
}

I guess I'm mainly interested in two things:

  1. Does this code do what I think it does (listen for connections, kill connections in case nothing happened for 5 sec and don't leak any memory)?
  2. It is more-or-less how is Asio supposed to be used? Or should I do something differently?
\$\endgroup\$
  • \$\begingroup\$ When I see: ios_base::sync_with_stdio(false); I usually expect to see std::cout.tie(nullptr); Otherwise read from std::cin force a flush on std::cout which is probably not what you want. \$\endgroup\$ – Martin York Sep 9 '16 at 20:28
  • \$\begingroup\$ I don't use std::cin and don't plan to. This program won't that any user input (well, over std::cin at least), does it still matter in that case? \$\endgroup\$ – graywolf Sep 9 '16 at 22:07
  • \$\begingroup\$ No it won't matter. But then why do you have ios_base::sync_with_stdio(false) The only reason to use this is to decouple the C++ and C standard input and output from each other which is only done if you want to make things faster when printing to these streams. \$\endgroup\$ – Martin York Sep 9 '16 at 22:33
  • \$\begingroup\$ I will potentially write a lot of stuff to std::cout, but I will not touch std::cin \$\endgroup\$ – graywolf Sep 9 '16 at 22:47
2
\$\begingroup\$

I see a number of things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every module in every program is a bad habit that you'd do well to avoid.

Reconsider the complex include guards

When I see something like this:

#ifndef SERVER_H_SFH9W7WB

I wonder, "what is the significance of SFH9W7WB?" I am guessing it's an autogenerated suffix to try to avoid name clashes, but I find it distracting and it is, in this code, also not needed since there are no name clashes.

Reconsider embedding the io_service in the server object

The current code has one io_service object per server, but that's not really necessary. A single io_service can serve multiple server objects. If you're worried about multithreading, reassure yourself by reading this: http://www.boost.org/doc/libs/1_61_0/doc/html/boost_asio/overview/core/threads.html

Prefer lambdas to boost::placeholders

Instead of writing something like this:

acceptor_.async_accept(
    new_con->socket(),
    bind(&server::handle_accept, this, new_con, placeholders::_1)
);

You could instead use a lambda, and perhaps eliminate handle_except entirely. See this page of code diffs to see what's different when using C++11 and lambdas.

Provide a means to gracefully exit the program

Other than killing the process, there's not a way to gracefully exit the program. One way I've done this in the past is to use something like this

int main() {
    std::thread myServer{serve};
    std::string command;
    std::cout << "Enter the word \"quit\" to exit the program and shut down the server\n";
    while (!done && std::cin >> command) {
        if (command == "quit") {
            done = true;
        }
    }
    myServer.join();
}

The serve function in this case is just a simple function to run the server and monitor the static boolean variable done.

Consider making a log object

Rather than having four discrete functions for the log functions, consider making a log object which could store a reference to the desired stream. This would then also allow for very simple logging to a file without having to change a lot of code.

Let the compiler generate code

Instead of including an empty constructor like this:

connection_manager::connection_manager() {}

Let the compiler automatically generate the code instead. If you want to make it very explicit that you're using the default constructor, you can put this in the declaration:

connection_manager() = default;

Don't use sync_with_stdio unless needed

It's not technically wrong, but as was already mentioned in the comments, use of sync_with_stdio within main doesn't really appear to be useful, and therefore should be omitted.

General advice

If you're just starting to learn asio, I would strongly recommend working through the tutorial, one piece at a time, until you really understand how the pieces all fit together. I'd also highly recommend the book C++ Concurrency in Action by Anthony Williams. There are a number of useful constructs in C++ that allow for multithreading (e.g. the shared_from_this already used within your code) but it's also very easy to get it slightly wrong and introduce a hard-to-find bug that only manifests itself occasionally (e.g. specifying a callback that's a member function for an object that may be deleted by the time the callback is invoked). I don't see such an error in your code, but I also didn't invest the time required to find such an error, should it exist.

Generally speaking, I'd recommend thinking carefully about when objects are created and destroyed. I often find that a little instrumentation in the code can help a lot with verifying your understanding.

\$\endgroup\$
  • \$\begingroup\$ I really appreciate your answer, thank you. I would just like to ask about "graceful exit" part of you answer. I thought that that is what "signals_.add(SIGTERM);" is supposed to do? At least my program seems to exit normally (prints closed socket etc.) while stopping it with CTRL+C \$\endgroup\$ – graywolf Sep 12 '16 at 14:17
  • 1
    \$\begingroup\$ Yes, perhaps I should have been more clear. Your signal handling code does clean things up, but there is no way out except via a signal. I find that a simple command line "quit" command is often very handy, as when automating testing or doing memory leak checks with valgrind. \$\endgroup\$ – Edward Sep 12 '16 at 14:21

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.