5
\$\begingroup\$

I recently wrote a tiny network library in C++17 called Turtle on Linux and wish to seek some improvement advice on how to further develop it. Any insights would be appreciated.

Origin: As a student, I took a class in Computer Network this semester, during which I practised serveral network programming projects in C using TCP socket. I thought it would be a good idea to wrap the meticulously detailed socket operations into C++ classes for reusability. And here I go.

Repo: GitHub Repo Link I've uploaded the little project to my github repo for reference. It is under the "Code-Review" branch that will stay static for a while to be reviewed. It contains a pretty detailed README. But I will illustrate more below anyway.

Design Highlight:

  • non-blocking socket and edge-trigger event handling mode
  • thread pool to execute tasks in a FIFO queuing manner
  • allow user custom server setup by only specifying 2 virtual callback functions

System Architecture:

  • the brain of the system is the Looper. It submits event-ready connection's callback function to ThreadPool to be executed, by doing epolling on the Poller.
  • the system starts with an Acceptor, which contains one acceptor connection which is under monitor of the Poller. The acceptor connection builds connection for each new incoming client.
  • the building block is Connection, which contains a Socket and a Buffer. Each client (as well as the listening acceptor) is essentially a Connection.

The diagram below briefly explains the flow.

Turtle System Diagram]

Codes:

There are quite a few helper classes. I will list them level by level up.

  1. Utils
#ifndef SRC_INCLUDE_UTILS_H_
#define SRC_INCLUDE_UTILS_H_

#define NON_COPYABLE(class_name)           \
  class_name(const class_name &) = delete; \
  class_name &operator=(const class_name &) = delete

#define NON_MOVEABLE(class_name)      \
  class_name(class_name &&) = delete; \
  class_name &operator=(class_name &&) = delete

#define NON_COPYABLE_AND_MOVEABLE(class_name)         \
  class_name(const class_name &) = delete;            \
  class_name &operator=(const class_name &) = delete; \
  class_name(class_name &&) = delete;                 \
  class_name &operator=(class_name &&) = delete

#endif  // SRC_INCLUDE_UTILS_H_
  1. Buffer
#ifndef SRC_INCLUDE_BUFFER_H_
#define SRC_INCLUDE_BUFFER_H_

#include <string>
#include <vector>

#define INITIAL_BUFFER_CAPACITY 1024

namespace TURTLE_SERVER {

/**
 * This Buffer abstracts an underlying dynamic char array
 * that allows pushing in byte data from two ends
 * NOT thread-safe
 * */
class Buffer {
  friend class Connection;

 public:
  explicit Buffer(size_t initial_capacity = INITIAL_BUFFER_CAPACITY);

  ~Buffer() = default;

  void Append(const char *new_char_data, size_t data_size);

  void Append(const std::string &new_str_data);

  void AppendHead(const char *new_char_data, size_t data_size);

  void AppendHead(const std::string &new_str_data);

  auto Size() const -> size_t;

  auto ToCString() -> char *;

  auto ToString() const -> std::string;

  void Clear();

 private:
  std::vector<char> buf_;
};

Buffer::Buffer(size_t initial_capacity) { buf_.reserve(initial_capacity); }

void Buffer::Append(const char *new_char_data, size_t data_size) {
  buf_.insert(buf_.end(), new_char_data, new_char_data + data_size);
}

void Buffer::Append(const std::string &new_str_data) {
  Append(new_str_data.c_str(), new_str_data.size());
}

void Buffer::AppendHead(const char *new_char_data, size_t data_size) {
  buf_.insert(buf_.begin(), new_char_data, new_char_data + data_size);
}

void Buffer::AppendHead(const std::string &new_str_data) {
  AppendHead(new_str_data.c_str(), new_str_data.size());
}

auto Buffer::Size() const -> size_t { return buf_.size(); }

auto Buffer::ToCString() -> char * {
  return reinterpret_cast<char *>(buf_.data());
}

auto Buffer::ToString() const -> std::string {
  return {buf_.begin(), buf_.end()};
}

void Buffer::Clear() { buf_.clear(); }

}  // namespace TURTLE_SERVER
#endif  // SRC_INCLUDE_BUFFER_H_
  1. NetAddress
#ifndef SRC_INCLUDE_NET_ADDRESS_H_
#define SRC_INCLUDE_NET_ADDRESS_H_

#include <arpa/inet.h>

#include <cstring>
#include <iostream>
#include <string>

namespace TURTLE_SERVER {

/**
 * This NetAddress class encapsulates the unique identifier of a network host
 * in the form of "IP Address + Port"
 * This class is compatible with both IPv4 and IPv6
 * */
class NetAddress {
 public:
  explicit NetAddress(bool is_ipv4 = true);

  NetAddress(const char *ip, uint16_t port, bool is_ipv4 = true);

  ~NetAddress() = default;

  auto IsIpv4() const -> bool { return is_ipv4_; }

  auto YieldAddr() -> struct sockaddr * { return &addr_; };

  auto YieldAddrLen() -> socklen_t * { return &addr_len_; };

  auto GetIp() const -> std::string;

  auto GetPort() const -> uint16_t;

  auto ToString() const -> std::string;

  friend std::ostream &operator<<(std::ostream &os, const NetAddress &address);

 private:
  const bool is_ipv4_;
  mutable struct sockaddr addr_ {};
  socklen_t addr_len_;
};

NetAddress::NetAddress(bool is_ipv4) : is_ipv4_(is_ipv4) {
  memset(&addr_, 0, sizeof(addr_));
  addr_len_ = sizeof(addr_);
}

NetAddress::NetAddress(const char *ip, uint16_t port, bool is_ipv4)
    : is_ipv4_(is_ipv4) {
  memset(&addr_, 0, sizeof(addr_));
  addr_len_ = sizeof(addr_);
  if (is_ipv4) {
    auto addr_ipv4 = reinterpret_cast<struct sockaddr_in *>(&addr_);
    addr_ipv4->sin_family = AF_INET;
    inet_pton(AF_INET, ip, &addr_ipv4->sin_addr.s_addr);
    addr_ipv4->sin_port = htons(port);
  } else {
    auto addr_ipv6 = reinterpret_cast<struct sockaddr_in6 *>(&addr_);
    addr_ipv6->sin6_family = AF_INET6;
    inet_pton(AF_INET6, ip, &addr_ipv6->sin6_addr.s6_addr);
    addr_ipv6->sin6_port = htons(port);
  }
}

auto NetAddress::GetIp() const -> std::string {
  char ip_address[INET6_ADDRSTRLEN];  // long enough for both Ipv4 and Ipv6
  if (is_ipv4_) {
    auto addr_ipv4 = reinterpret_cast<struct sockaddr_in *>(&addr_);
    inet_ntop(AF_INET, &addr_ipv4->sin_addr, ip_address, INET_ADDRSTRLEN);
  } else {
    auto addr_ipv6 = reinterpret_cast<struct sockaddr_in6 *>(&addr_);
    inet_ntop(AF_INET6, &addr_ipv6->sin6_addr, ip_address, INET6_ADDRSTRLEN);
  }
  return ip_address;
}

auto NetAddress::GetPort() const -> uint16_t {
  uint16_t port;
  if (is_ipv4_) {
    auto addr_ipv4 = reinterpret_cast<struct sockaddr_in *>(&addr_);
    port = ntohs(addr_ipv4->sin_port);
  } else {
    auto addr_ipv6 = reinterpret_cast<struct sockaddr_in6 *>(&addr_);
    port = ntohs(addr_ipv6->sin6_port);
  }
  return port;
}

auto NetAddress::ToString() const -> std::string {
  return GetIp() + std::string(" @ ") + std::to_string(GetPort());
}

std::ostream &operator<<(std::ostream &os, const NetAddress &address) {
  os << address.ToString();
  return os;
}
}  // namespace TURTLE_SERVER
#endif  // SRC_INCLUDE_NET_ADDRESS_H_
  1. Socket
#ifndef SRC_INCLUDE_SOCKET_H_
#define SRC_INCLUDE_SOCKET_H_

#include <fcntl.h>
#include <sys/socket.h>
#include <unistd.h>

#include <cassert>
#include <cerrno>

#include "net_address.h"
#include "utils.h"

namespace TURTLE_SERVER {

/**
 * This Socket class encapsulates a socket descriptor
 * which can act as either listener or client
 * This class is compatible with both IPv4 and IPv6
 * */
class Socket {
 public:
  explicit Socket(bool is_ipv4 = true);

  explicit Socket(int fd);

  NON_COPYABLE(Socket);

  Socket(Socket &&other) noexcept;

  Socket &operator=(Socket &&other) noexcept;

  ~Socket();

  auto GetFd() const -> int;

  /* for client, one step: directly connect */
  void Connect(NetAddress &server_address);  // NOLINT

  /* for server, three steps: bind + listen + accept */
  void Bind(NetAddress &server_address);  // NOLINT

  void Listen();

  auto Accept(NetAddress &client_address) -> int;  // NOLINT

  void SetReusable();

  void SetNonBlocking();

 private:
  int fd_{-1};
};

Socket::Socket(bool is_ipv4) : fd_(-1) {
  if (is_ipv4) {
    fd_ = socket(AF_INET, SOCK_STREAM, 0);
  } else {
    fd_ = socket(AF_INET6, SOCK_STREAM, 0);
  }
  if (fd_ == -1) {
    perror("Socket: socket() error");
    exit(EXIT_FAILURE);
  }
}

Socket::Socket(int fd) : fd_(fd) {}

Socket::Socket(Socket &&other) noexcept {
  fd_ = other.fd_;
  other.fd_ = -1;
}

Socket &Socket::operator=(Socket &&other) noexcept {
  if (fd_ != -1) {
    close(fd_);
  }
  fd_ = other.fd_;
  other.fd_ = -1;
  return *this;
}

Socket::~Socket() {
  if (fd_ != -1) {
    close(fd_);
    fd_ = -1;
  }
}

auto Socket::GetFd() const -> int { return fd_; }

void Socket::Connect(NetAddress &server_address) {
  assert(fd_ != -1 && "cannot Connect() with an invalid fd");
  if (connect(fd_, server_address.YieldAddr(),
              *server_address.YieldAddrLen()) == -1) {
    perror("Socket: Connect() error");
    exit(EXIT_FAILURE);
  }
}

void Socket::Bind(NetAddress &server_address) {
  assert(fd_ != -1 && "cannot Bind() with an invalid fd");
  if (bind(fd_, server_address.YieldAddr(), *server_address.YieldAddrLen()) ==
      -1) {
    perror("Socket: Bind() error");
    exit(EXIT_FAILURE);
  }
}

void Socket::Listen() {
  assert(fd_ != -1 && "cannot Listen() with an invalid fd");
  if (listen(fd_, BACK_LOG) == -1) {
    perror("Socket: Listen() error");
    exit(EXIT_FAILURE);
  }
}

auto Socket::Accept(NetAddress &client_address) -> int {
  assert(fd_ != -1 && "cannot Accept() with an invalid fd");
  int client_fd = -1;
  if ((client_fd = accept(fd_, client_address.YieldAddr(),
                          client_address.YieldAddrLen())) == -1) {
    perror("Socket: Accept() error");
    exit(EXIT_FAILURE);
  }
  return client_fd;
}

void Socket::SetReusable() {
  assert(fd_ != -1 && "cannot SetReusable() with an invalid fd");
  int yes = 1;
  if (setsockopt(fd_, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof yes) == -1 ||
      setsockopt(fd_, SOL_SOCKET, SO_REUSEPORT, &yes, sizeof yes) == -1) {
    perror("Socket: SetReusable() error");
    exit(EXIT_FAILURE);
  }
}

void Socket::SetNonBlocking() {
  assert(fd_ != -1 && "cannot SetNonBlocking() with an invalid fd");
  if (fcntl(fd_, F_SETFL, fcntl(fd_, F_GETFL) | O_NONBLOCK) == -1) {
    perror("Socket: SetNonBlocking() error");
    exit(EXIT_FAILURE);
  }
}
}  // namespace TURTLE_SERVER
#endif  // SRC_INCLUDE_SOCKET_H_
  1. Connnetion
#ifndef SRC_INCLUDE_CONNECTION_H_
#define SRC_INCLUDE_CONNECTION_H_

#include <functional>
#include <memory>
#include <string>
#include <utility>

#include "buffer.h"
#include "socket.h"
#include "utils.h"

#define TEMP_BUF_SIZE 2048

namespace TURTLE_SERVER {

// forward declaration
class Looper;

/**
 * This Connection class encapsulates a TCP client connection
 * It could be set a custom callback function when new messages arrive
 * and it contains information about the monitoring events and return events
 * so that Poller could manipulate and epoll based on this Connection class
 * */
class Connection {
 public:
  explicit Connection(Looper *looper, std::unique_ptr<Socket> socket);
  ~Connection() = default;

  NON_COPYABLE(Connection);

  auto GetFd() const -> int;
  auto GetSocket() -> Socket *;

  /* for Poller */
  void SetEvents(uint32_t events);
  auto GetEvents() const -> uint32_t;
  void SetRevents(uint32_t revents);
  auto GetRevents() const -> uint32_t;

  void SetInPoller(bool in_poller);
  auto InPoller() const -> bool;
  void SetCallback(const std::function<void(Connection *)> &callback);
  auto GetCallback() -> std::function<void()>;

  auto GetLooper() -> Looper *;

  /* for Buffer */
  auto GetReadBuffer() -> Buffer *;
  auto GetWriteBuffer() -> Buffer *;
  auto GetReadBufferSize() -> size_t;
  auto GetWriteBufferSize() -> size_t;
  void WriteToReadBuffer(const char *buf, size_t size);
  void WriteToWriteBuffer(const char *buf, size_t size);
  void WriteToReadBuffer(const std::string &str);
  void WriteToWriteBuffer(const std::string &str);

  auto Read() -> const char *;
  auto ReadAsString() const -> std::string;

  /* return std::pair<How many bytes read, whether the client exits> */
  auto Recv() -> std::pair<ssize_t, bool>;
  void Send();
  void ClearReadBuffer();
  void ClearWriteBuffer();

 private:
  std::unique_ptr<Buffer> read_buffer_;
  std::unique_ptr<Buffer> write_buffer_;
  std::unique_ptr<Socket> socket_;
  Looper *looper_;
  bool in_poller_{false};
  uint32_t events_{};
  uint32_t revents_{};
  std::function<void()> callback_{nullptr};
};


Connection::Connection(Looper *looper, std::unique_ptr<Socket> socket)
    : looper_(looper),
      socket_(std::move(socket)),
      read_buffer_(std::make_unique<Buffer>()),
      write_buffer_(std::make_unique<Buffer>()),
      events_(0),
      revents_(0) {}

auto Connection::GetFd() const -> int { return socket_->GetFd(); }

auto Connection::GetSocket() -> Socket * { return socket_.get(); }

void Connection::SetEvents(uint32_t events) { events_ = events; }

auto Connection::GetEvents() const -> uint32_t { return events_; }

void Connection::SetRevents(uint32_t revents) { revents_ = revents; }

auto Connection::GetRevents() const -> uint32_t { return revents_; }

void Connection::SetInPoller(bool in_poller) { in_poller_ = in_poller; }

auto Connection::InPoller() const -> bool { return in_poller_; }

void Connection::SetCallback(
    const std::function<void(Connection *)> &callback) {
  callback_ = [callback, this] { return callback(this); };
}

auto Connection::GetCallback() -> std::function<void()> { return callback_; }

auto Connection::GetLooper() -> Looper * { return looper_; }

auto Connection::GetReadBuffer() -> Buffer * { return read_buffer_.get(); }

auto Connection::GetWriteBuffer() -> Buffer * { return write_buffer_.get(); }

auto Connection::GetReadBufferSize() -> size_t { return read_buffer_->Size(); }

auto Connection::GetWriteBufferSize() -> size_t {
  return write_buffer_->Size();
}

void Connection::WriteToReadBuffer(const char *buf, size_t size) {
  read_buffer_->Append(buf, size);
}

void Connection::WriteToWriteBuffer(const char *buf, size_t size) {
  write_buffer_->Append(buf, size);
}

void Connection::WriteToReadBuffer(const std::string &str) {
  read_buffer_->Append(str);
}

void Connection::WriteToWriteBuffer(const std::string &str) {
  write_buffer_->Append(str);
}

auto Connection::Read() -> const char * { return read_buffer_->buf_.data(); }

auto Connection::ReadAsString() const -> std::string {
  return read_buffer_->ToString();
}

auto Connection::Recv() -> std::pair<ssize_t, bool> {
  // read all available bytes, since Edge-trigger
  int from_fd = GetFd();
  ssize_t read = 0;
  char buf[TEMP_BUF_SIZE + 1];
  memset(buf, 0, sizeof(buf));
  while (true) {
    ssize_t curr_read = recv(from_fd, buf, TEMP_BUF_SIZE, 0);
    if (curr_read > 0) {
      read += curr_read;
      WriteToReadBuffer(buf, curr_read);
      memset(buf, 0, sizeof(buf));
    } else if (curr_read == 0) {
      // the client has exit
      std::cout << "Client exits: " << from_fd << std::endl;
      return {read, true};
    } else if (curr_read == -1 && errno == EINTR) {
      // normal interrupt
      continue;
    } else if (curr_read == -1 && (errno == EAGAIN || errno == EWOULDBLOCK)) {
      // all data read
      break;
    } else {
      perror("HandleConnection: recv() error");
      exit(EXIT_FAILURE);
    }
  }
  return {read, false};
}

void Connection::Send() {
  // robust write
  size_t curr_write = 0;
  size_t write;
  const size_t to_write = GetWriteBufferSize();
  const char *buf = write_buffer_->buf_.data();
  while (curr_write < to_write) {
    if ((write = send(GetFd(), buf + curr_write, to_write - curr_write, 0)) <=
        0) {
      if (errno != EINTR) {
        perror("Send(): send error and error is not EINTR");
        exit(EXIT_FAILURE);
      }
      write = 0;
    }
    curr_write += write;
  }
  ClearWriteBuffer();
}

void Connection::ClearReadBuffer() { read_buffer_->Clear(); }

void Connection::ClearWriteBuffer() { write_buffer_->Clear(); }

}  // namespace TURTLE_SERVER
#endif  // SRC_INCLUDE_CONNECTION_H_
  1. Poller
#ifndef SRC_INCLUDE_POLLER_H_
#define SRC_INCLUDE_POLLER_H_

#include <sys/epoll.h>

#include <memory>
#include <vector>

#include "connection.h"
#include "utils.h"

/* the default maximum number of events to be listed on epoll tree */
#define DEFAULT_EVENTS_LISTENED 1024

namespace TURTLE_SERVER {

/**
 * This Poller acts at the socket monitor that actively polling on connections
 * */
class Poller {
 public:
  explicit Poller(uint64_t poll_size = DEFAULT_EVENTS_LISTENED);

  ~Poller();

  NON_COPYABLE(Poller);

  void AddConnection(Connection *conn);

  auto Poll(int timeout = -1) -> std::vector<Connection *>;

  auto GetPollSize() const -> uint64_t;

 private:
  int poll_fd_;
  struct epoll_event *poll_events_{nullptr};
  uint64_t poll_size_;
};

Poller::Poller(uint64_t poll_size) : poll_size_(poll_size) {
  poll_fd_ = epoll_create1(0);
  if (poll_fd_ == -1) {
    perror("Poller: epoll_create1() error");
    exit(EXIT_FAILURE);
  }
  poll_events_ = new struct epoll_event[poll_size];
  memset(poll_events_, 0, poll_size_ * sizeof(struct epoll_event));
}

Poller::~Poller() {
  if (poll_fd_ != -1) {
    close(poll_fd_);
    delete[] poll_events_;
    poll_fd_ = -1;
  }
}

void Poller::AddConnection(Connection *conn) {
  assert(conn->GetFd() != -1 && "cannot AddConnection() with an invalid fd");
  struct epoll_event event;
  memset(&event, 0, sizeof(struct epoll_event));
  event.data.ptr = conn;
  event.events = conn->GetEvents();
  int ret_val = epoll_ctl(poll_fd_, EPOLL_CTL_ADD, conn->GetFd(), &event);
  if (ret_val == -1) {
    perror("Poller: epoll_ctl add error");
    exit(EXIT_FAILURE);
  }
  conn->SetInPoller(true);
}

auto Poller::Poll(int timeout) -> std::vector<Connection *> {
  std::vector<Connection *> events_happen;
  int ready = epoll_wait(poll_fd_, poll_events_, poll_size_, timeout);
  if (ready == -1) {
    perror("Poller: Poll() error");
    exit(EXIT_FAILURE);
  }
  for (int i = 0; i < ready; i++) {
    Connection *ready_connection =
        reinterpret_cast<Connection *>(poll_events_[i].data.ptr);
    ready_connection->SetRevents(poll_events_[i].events);
    events_happen.emplace_back(ready_connection);
  }
  return events_happen;
}

auto Poller::GetPollSize() const -> uint64_t { return poll_size_; }
}  // namespace TURTLE_SERVER
#endif  // SRC_INCLUDE_POLLER_H_
  1. Looper
#ifndef SRC_INCLUDE_LOOPER_H_
#define SRC_INCLUDE_LOOPER_H_

#include <atomic>
#include <functional>
#include <future>  // NOLINT
#include <map>
#include <memory>
#include <mutex>  // NOLINT

#include "acceptor.h"
#include "connection.h"
#include "poller.h"
#include "thread_pool.h"
#include "utils.h"
/* the epoll_wait time in milliseconds */
#define TIMEOUT 3000

namespace TURTLE_SERVER {

class Acceptor;
/**
 * This Looper acts as the central coordinator between executor (ThreadPool) and
 * event polling (Poller)
 * */
class Looper {
 public:
  explicit Looper(ThreadPool *pool);

  ~Looper() = default;

  NON_COPYABLE(Looper);

  void Loop();

  void AddAcceptor(std::unique_ptr<Acceptor> acceptor);

  void AddConnection(std::unique_ptr<Connection> new_conn);

  auto DeleteConnection(int fd) -> bool;

  auto DispatchTask(const std::function<void()> &task) -> std::future<void>;

  auto GetAcceptor() -> Acceptor *;

  void Exit();

 private:
  ThreadPool *pool_;
  std::unique_ptr<Poller> poller_;
  std::mutex mtx_;
  std::unique_ptr<Acceptor> acceptor_{nullptr};
  std::map<int, std::unique_ptr<Connection>> connections_;
  std::atomic<bool> exit_{false};
};

Looper::Looper(ThreadPool *pool)
    : poller_(std::make_unique<Poller>()), pool_(pool) {}

void Looper::Loop() {
  if (!acceptor_) {
    throw std::runtime_error(
        "Looper: Loop() called before setting up acceptor");
  }
  while (!exit_) {
    auto ready_connections = poller_->Poll(TIMEOUT);
    for (auto &conn : ready_connections) {
      auto fut = DispatchTask(conn->GetCallback());
    }
  }
}

void Looper::AddAcceptor(std::unique_ptr<Acceptor> acceptor) {
  acceptor_ = std::move(acceptor);
  std::unique_lock<std::mutex> lock(mtx_);
  poller_->AddConnection(acceptor_->GetAcceptorConnection());
}

void Looper::AddConnection(std::unique_ptr<Connection> new_conn) {
  std::unique_lock<std::mutex> lock(mtx_);
  poller_->AddConnection(new_conn.get());
  int fd = new_conn->GetFd();
  connections_.insert({fd, std::move(new_conn)});
}

auto Looper::DeleteConnection(int fd) -> bool {
  std::unique_lock<std::mutex> lock(mtx_);
  auto it = connections_.find(fd);
  if (it == connections_.end()) {
    return false;
  }
  connections_.erase(it);
  return true;
}

auto Looper::DispatchTask(const std::function<void()> &task)
    -> std::future<void> {
  return pool_->SubmitTask(task);
}

auto Looper::GetAcceptor() -> Acceptor * { return acceptor_.get(); }

void Looper::Exit() { exit_ = true; }
}  // namespace TURTLE_SERVER
#endif  // SRC_INCLUDE_LOOPER_H_
  1. Acceptor
#ifndef SRC_INCLUDE_ACCEPTOR_H_
#define SRC_INCLUDE_ACCEPTOR_H_

#include <functional>
#include <memory>

#include "connection.h"
#include "looper.h"
#include "net_address.h"
#include "utils.h"

namespace TURTLE_SERVER {

/**
 * This Acceptor comes with basic functionality for accepting new client
 * connections and add its into the Poller More custom handling could be added
 * as well
 * */
class Acceptor {
 public:
  explicit Acceptor(Looper *looper, NetAddress server_address);

  ~Acceptor() = default;

  NON_COPYABLE(Acceptor);

  void BaseAcceptCallback(Connection *server_conn);

  void SetCustomAcceptCallback(
      std::function<void(Connection *)> custom_accept_callback);

  void SetCustomHandleCallback(
      std::function<void(Connection *)> custom_handle_callback);

  auto GetCustomAcceptCallback() -> std::function<void(Connection *)>;

  auto GetCustomHandleCallback() -> std::function<void(Connection *)>;

  auto GetAcceptorConnection() -> Connection *;

 private:
  Looper *looper_;
  std::unique_ptr<Connection> acceptor_conn;
  std::function<void(Connection *)> custom_accept_callback_{};
  std::function<void(Connection *)> custom_handle_callback_{};
};

Acceptor::Acceptor(Looper *looper, NetAddress server_address)
    : looper_(looper) {
  auto acceptor_sock = std::make_unique<Socket>();
  acceptor_sock->SetReusable();
  acceptor_sock->Bind(server_address);
  acceptor_sock->Listen();
  acceptor_conn =
      std::make_unique<Connection>(looper_, std::move(acceptor_sock));
  acceptor_conn->SetEvents(EPOLLIN);  // not edge-trigger for listener
  SetCustomAcceptCallback({});
  SetCustomHandleCallback({});
}

/*
 * basic functionality for accepting new connection
 * provided to the acceptor by default
 */
void Acceptor::BaseAcceptCallback(Connection *server_conn) {
  NetAddress client_address;
  int accept_fd = server_conn->GetSocket()->Accept(client_address);
  std::cout << "New client joins: " << accept_fd << std::endl;
  auto client_sock = std::make_unique<Socket>(accept_fd);
  client_sock->SetNonBlocking();
  auto client_connection = std::make_unique<Connection>(
      server_conn->GetLooper(), std::move(client_sock));
  client_connection->SetEvents(EPOLLIN | EPOLLET);  // edge-trigger for client
  client_connection->SetCallback(GetCustomHandleCallback());
  server_conn->GetLooper()->AddConnection(std::move(client_connection));
}
void Acceptor::SetCustomAcceptCallback(
    std::function<void(Connection *)> custom_accept_callback) {
  custom_accept_callback_ = std::move(custom_accept_callback);
  acceptor_conn->SetCallback([this](auto &&PH1) {
    BaseAcceptCallback(std::forward<decltype(PH1)>(PH1));
    GetCustomAcceptCallback()(std::forward<decltype(PH1)>(PH1));
  });
}

void Acceptor::SetCustomHandleCallback(
    std::function<void(Connection *)> custom_handle_callback) {
  custom_handle_callback_ = std::move(custom_handle_callback);
}

auto Acceptor::GetCustomAcceptCallback() -> std::function<void(Connection *)> {
  return custom_accept_callback_;
}

auto Acceptor::GetCustomHandleCallback() -> std::function<void(Connection *)> {
  return custom_handle_callback_;
}

auto Acceptor::GetAcceptorConnection() -> Connection * {
  return acceptor_conn.get();
}
}  // namespace TURTLE_SERVER

#endif  // SRC_INCLUDE_ACCEPTOR_H_
  1. ThreadPool
#include <algorithm>
#include <atomic>
#include <condition_variable>  // NOLINT
#include <functional>
#include <future>  // NOLINT
#include <memory>
#include <mutex>  // NOLINT
#include <queue>
#include <thread>  // NOLINT
#include <utility>
#include <vector>

#include "utils.h"
#ifndef SRC_INCLUDE_THREAD_POOL_H_
#define SRC_INCLUDE_THREAD_POOL_H_

/* The minimum number of threads to exist in the threadpool */
#define MIN_NUM_THREADS_IN_POOL 2

namespace TURTLE_SERVER {

/**
 * This ThreadPool manages the thread resources and acts as the executor for
 * client requests upon submitting a task, it gives back a future to be waited
 * for
 * */
class ThreadPool {
 public:
  explicit ThreadPool(int size = std::thread::hardware_concurrency());

  ~ThreadPool();

  NON_COPYABLE(ThreadPool);

  template <typename F, typename... Args>
  decltype(auto) SubmitTask(F &&new_task, Args &&...args);

  void Exit();

 private:
  std::vector<std::thread> threads_;
  std::queue<std::function<void()>> tasks_;
  std::mutex mtx_;
  std::condition_variable cv_;
  std::atomic<bool> exit_{false};
};

template <typename F, typename... Args>
decltype(auto) ThreadPool::SubmitTask(F &&new_task, Args &&...args) {
  using return_type = std::invoke_result_t<F, Args...>;
  if (exit_) {
    throw std::runtime_error(
        "ThreadPool: SubmitTask() called while already exit_ being true");
  }
  auto packaged_new_task = std::make_shared<std::packaged_task<return_type()>>(
      std::bind(std::forward<F>(new_task), std::forward<Args>(args)...));
  auto fut = packaged_new_task->get_future();
  {
    // submit in form of std::function to the Thread Pool task queue
    std::unique_lock<std::mutex> lock(mtx_);
    tasks_.emplace([packaged_new_task]() { (*packaged_new_task)(); });
  }
  cv_.notify_one();
  return fut;
}

ThreadPool::ThreadPool(int size) {
  /* std::thread::hardware_concurrency() might return 0 if sys info not
   * available */
  size = std::max(size, MIN_NUM_THREADS_IN_POOL);
  for (auto i = 0; i < size; i++) {
    threads_.emplace_back([this]() {
      while (true) {
        std::function<void()> next_task;
        {
          std::unique_lock<std::mutex> lock(mtx_);
          cv_.wait(lock, [this]() { return exit_ || !tasks_.empty(); });
          if (exit_ && tasks_.empty()) {
            return;  // thread life ends
          }
          next_task = tasks_.front();
          tasks_.pop();
        }
        next_task();
      }
    });
  }
}

ThreadPool::~ThreadPool() {
  Exit();
  for (auto &worker : threads_) {
    if (worker.joinable()) {
      worker.join();
    }
  }
}

void ThreadPool::Exit() {
  exit_ = true;
  cv_.notify_all();
}
}  // namespace TURTLE_SERVER

#endif  // SRC_INCLUDE_THREAD_POOL_H_
  1. TurtleServer base class interface
#include <memory>
#include <utility>

#include "acceptor.h"
#include "connection.h"
#include "looper.h"
#include "net_address.h"
#include "poller.h"
#include "socket.h"
#include "thread_pool.h"
#include "utils.h"

#ifndef SRC_INCLUDE_TURTLE_SERVER_H_
#define SRC_INCLUDE_TURTLE_SERVER_H_

namespace TURTLE_SERVER {

/**
 * The base class for setting up a web server using the Turtle framework
 * User should design a class that inherits from the TurtleServer base class
 * and implements the two virtual function 'OnHandle' and 'OnAccept'
 * The rest is already taken care of and in most cases users don't need to touch
 * upon
 *
 * OnAccept(): Given the acceptor connection, when the Poller tells us there is
 * new incoming client connection basic step of socket accept and build
 * connection and add into the Poller are already taken care of in the
 * Acceptor::BaseAcceptCallback. This OnAccept() functionality is appended to
 * that base BaseAcceptCallback and called after that base, to support any
 * custom business logic upon receiving new client connection
 *
 * OnHandle(): No base version exists. Users should implement this function to
 * achieve the expected behavior
 */
class TurtleServer {
 public:
  explicit TurtleServer(NetAddress server_address)
      : pool_(std::make_unique<ThreadPool>()),
        looper_(std::make_unique<Looper>(pool_.get())) {
    auto acceptor = std::make_unique<Acceptor>(looper_.get(), server_address);
    acceptor->SetCustomHandleCallback(
        [this](auto &&PH1) { OnHandle(std::forward<decltype(PH1)>(PH1)); });
    acceptor->SetCustomAcceptCallback(
        [this](auto &&PH1) { OnAccept(std::forward<decltype(PH1)>(PH1)); });
    looper_->AddAcceptor(std::move(acceptor));
  }

  virtual ~TurtleServer() = default;

  /* Not Edge trigger */
  virtual void OnAccept(Connection *acceptor_conn) = 0;

  /* Edge trigger! Read all bytes please */
  virtual void OnHandle(Connection *client_conn) = 0;

  virtual void Begin() { looper_->Loop(); }

  auto GetPool() -> ThreadPool * { return pool_.get(); }
  auto GetLooper() -> Looper * { return looper_.get(); }

 private:
  std::unique_ptr<ThreadPool> pool_;
  std::unique_ptr<Looper> looper_;
};
}  // namespace TURTLE_SERVER

#endif  // SRC_INCLUDE_TURTLE_SERVER_H_

Demo

A demo echo server could be setup fairly easy in less than 30 lines of real code:

#include "turtle_server.h"

namespace TURTLE_SERVER {

class EchoServer : public TurtleServer {
 public:
  explicit EchoServer(NetAddress server_address)
      : TurtleServer(server_address) {}

  void OnAccept(Connection *server_conn) final {}

  void OnHandle(Connection *client_conn) final {
    int from_fd = client_conn->GetFd();
    auto [read, exit] = client_conn->Recv();
    if (exit) {
      client_conn->GetLooper()->DeleteConnection(from_fd);
      // client_conn ptr is destoryed and invalid below here, do not touch it
      // again
      return;
    }
    if (read) {
      client_conn->WriteToWriteBuffer(client_conn->ReadAsString());
      client_conn->Send();
      client_conn->ClearReadBuffer();
    }
  }
};

}  // namespace TURTLE_SERVER

int main() {
  TURTLE_SERVER::NetAddress local_address("0.0.0.0", 20080);
  TURTLE_SERVER::EchoServer echo_server(local_address);
  echo_server.Begin();
  return 0;
}

Next Step Ideas

I am thinking about what to do for the next step to further improve my library. A few things pop up my mind:

  • Support HTTP Request parsing and responding (maybe just GET method first)
  • Add a Cache layer after able to server HTTP GET Request
  • Add performance test benchmark like web bench
  • Add Timer to kill long-time inactive client connections
  • Don't be lazy and add comprehensive Unit Test 😓

Any critique/advice/suggestions would be appreciated either on my current implementation or future next-step plans! I am pretty inexperienced in Networking and C++ language. There must be a lot of places I could learn to do more and do better!


Edit:

I've started to reflect and revise according to the kind suggestions I receive in another branch code-review-revision for people's reference.

I have just improved upon suggestions by rioki. And will continue to do so w.r.t to suggestions by pacmaninbw.

Since I recently drafted out an additional basic HTTP-GET only component of my framework, I think a lot of lessons I learned here could be applied to that component as well. I will try to do so.

\$\endgroup\$
7
  • \$\begingroup\$ We cannot review an architectural diagram or a repo link. Please paste inline code. \$\endgroup\$
    – Reinderien
    Dec 28, 2022 at 19:24
  • \$\begingroup\$ Oh I see. Sorry about that. I don't know it. But it would be quite a few files of code. Is that also allowed? \$\endgroup\$ Dec 28, 2022 at 19:35
  • \$\begingroup\$ Yes - the code limit is quite high :) \$\endgroup\$
    – Reinderien
    Dec 28, 2022 at 19:36
  • 2
    \$\begingroup\$ It's reviewable, though I suggest that you re-introduce the diagram and repo link for context. They're useful, just not on their own. My apologies; I should have been more clear on that point. \$\endgroup\$
    – Reinderien
    Dec 28, 2022 at 20:00
  • 2
    \$\begingroup\$ I think you might want to create a branch on GitHub that will always contain this code. The main branch would be where you can continue to do development. You can find examples of branching for the Code Review Community here. Among other things branching will allow you to fix bugs as new code in one branch and then transfer the fix to released code as a bug fix or a patch. \$\endgroup\$
    – pacmaninbw
    Dec 29, 2022 at 14:43

2 Answers 2

3
\$\begingroup\$
#ifndef SRC_INCLUDE_UTILS_H_
#define SRC_INCLUDE_UTILS_H_

#define NON_COPYABLE(class_name)           \
  class_name(const class_name &) = delete; \
  class_name &operator=(const class_name &) = delete

#define NON_MOVEABLE(class_name)      \
  class_name(class_name &&) = delete; \
  class_name &operator=(class_name &&) = delete

#define NON_COPYABLE_AND_MOVEABLE(class_name)         \
  class_name(const class_name &) = delete;            \
  class_name &operator=(const class_name &) = delete; \
  class_name(class_name &&) = delete;                 \
  class_name &operator=(class_name &&) = delete

#endif  // SRC_INCLUDE_UTILS_H_

You should try to avoid using macros. If you really can't write a deleted copy constructor and assignment operator, you should look into something like boost::noncpyable, it's really simple, you can put one in your code and it's proper C++.

The NON_MOVEABLE is actually useless, since classes that are non copyable are automatically non movable. If you want default move, you need to add explicit defaults.

Even through NON_COPYABLE_AND_MOVEABLE with an obsolete NON_MOVEABLE making NON_COPYABLE_AND_MOVEABLE obsolete; following the DRY principle, you should call lNON_COPYABLE and NON_MOVEABLE from NON_COPYABLE_AND_MOVEABLE.


#ifndef SRC_INCLUDE_BUFFER_H_
#define SRC_INCLUDE_BUFFER_H_

If you are using macro include guards, you should probably name them something unique. Adding the name of your library is a good idea, such as _TURTLE_BUFFER_H_. The chance is to high that SRC_INCLUDE_BUFFER_H_ is used by some other library.


#define INITIAL_BUFFER_CAPACITY 1024

Consider using constexpr for defaults and put them in your namespace.


namespace TURTLE_SERVER {

This namespace would drive me crazy. I prefer stating the namespace on every object. For example std::vector, glm::mat4 or c9y::sync. You notice how the namespaces are really short? I think turtle::Server is about as much as I could muster.

class Buffer {
  friend class Connection;

 public:
  // a few functions

 private:
  std::vector<char> buf_;
};

Why does Connection need access to Buffer's private parts. I think in the sense of logical modularization, connections use buffers, but are certainly not part of a common API.


class Buffer {
 public:
  explicit Buffer(size_t initial_capacity = INITIAL_BUFFER_CAPACITY);

  ~Buffer() = default;
};

I would add two constructors, one with the value and the other as default constructor. I find adding default values, which live in the calling code, hinder making small changes without ABI breaks.

Also if the constructor is copyable, I would add defaulted copy constructor and assignment operator, following the rule of five.

class Buffer {
 public:
  void Append(const char *new_char_data, size_t data_size);

 private:
  std::vector<char> buf_;
};

Since Buffer is almost certainly not handling text, consider using std::byte instead of char.


auto Buffer::ToCString() -> char * {
  return reinterpret_cast<char *>(buf_.data());
}

auto Buffer::ToString() const -> std::string {
  return {buf_.begin(), buf_.end()};
}

(Not a fan of trailing return type, but that's fine.)

You probably should also provide a const version of the function ToCString for read only access.

If you are interpreting the contents of the buffer as strings, consider providing a view as a string_view to prevent copying the data.


class NetAddress 

Think of the name in context, what other type of address exists in a networking library? Consider turtle::Address. (If you write the namesapce, you do not need namespace like bits in the name of objects.)


class NetAddress {
 public:
  explicit NetAddress(bool is_ipv4 = true);
  NetAddress(const char *ip, uint16_t port, bool is_ipv4 = true);
};

You probably should defined an enum named Protocol with the values IP4 and IP6. This makes the code simpler to understand at the call site, since NetAddress(true) is not explicit, compared to NetAddress(Protocol::IP4).

Also, why do calls need to specify the protocol at the default construction of the NetAddress?


std::ostream &operator<<(std::ostream &os, const NetAddress &address) {
  os << address.ToString();
  return os;
}

Since you already have a ToString function, consider defining the stream operator as standalone.


class Socket {
 public:
  explicit Socket(bool is_ipv4 = true);
  void Connect(NetAddress &server_address); 
  void Bind(NetAddress &server_address); 

};

Consider delaying the creation of the socket so that you can derive the protocol form the NetAddress.

Socket::Socket(bool is_ipv4) : fd_(-1) {
  if (is_ipv4) {
    fd_ = socket(AF_INET, SOCK_STREAM, 0);
  } else {
    fd_ = socket(AF_INET6, SOCK_STREAM, 0);
  }
  if (fd_ == -1) {
    perror("Socket: socket() error");
    exit(EXIT_FAILURE);
  }
}

This is C++, you probably should throw an exception here. You can later still to the perror and exit. But the client code of Socket may not agree with your error handling strategy here. (Applies to all other instance error handling.)

Socket &Socket::operator=(Socket &&other) noexcept {
  if (fd_ != -1) {
    close(fd_);
  }
  fd_ = other.fd_;
  other.fd_ = -1;
  return *this;
}

Properly implementing move is hard. There are different theories what is correct. But in this case I think swapping the fd_ and letting the destructor of the rvalue handle cleanup would be a viable strategy that reduces code duplication.

auto Socket::GetFd() const -> int { return fd_; }

This probably should be noexcept. You probably should have WAY more noexcept. Any function that has no sane way to throw an exception should have noexcept; your optimizer will thank you.


class Connection {
 public:
  explicit Connection(Looper *looper, std::unique_ptr<Socket> socket);
};

You put so much effort into making Socket movable and now you are using a unique_ptr? Both are viable strategies, but you should decide wich aproach you want to take. I would prefer unique_ptr, a tad more expensive, but at least you can't mess up any move semantics.

The lopper object is not used by the Connection, you probably should remove it.


class Connection {
 public:
  auto GetReadBuffer() -> Buffer *;
  auto GetWriteBuffer() -> Buffer *;
  auto GetReadBufferSize() -> size_t;
  auto GetWriteBufferSize() -> size_t;
  void WriteToReadBuffer(const char *buf, size_t size);
  void WriteToWriteBuffer(const char *buf, size_t size);
  void WriteToReadBuffer(const std::string &str);
  void WriteToWriteBuffer(const std::string &str);
};

Not sure if these function should be public. You should have a send / receive API that know nothing of buffers or the buffers are the bits moving though your system.

For example, either:

class Connection {
 public:
  void Send(const std::string& value);
  void Send(const unsigned int value);
  void Send(const float value);

  std::string ReciveString();
  unsigned int ReciveUint(const value);
  float ReciveFloat();
};

or:

class Connection {
 public:
  void Send(const Buffer& message);
  Buffer Recive();
};

Here an elegant way to build and decode a buffer may be of use.


class TurtleServer {
 public:
  explicit TurtleServer(NetAddress server_address)
      : pool_(std::make_unique<ThreadPool>()),
        looper_(std::make_unique<Looper>(pool_.get())) {
    auto acceptor = std::make_unique<Acceptor>(looper_.get(), server_address);
    acceptor->SetCustomHandleCallback(
        [this](auto &&PH1) { OnHandle(std::forward<decltype(PH1)>(PH1)); });
    acceptor->SetCustomAcceptCallback(
        [this](auto &&PH1) { OnAccept(std::forward<decltype(PH1)>(PH1)); });
    looper_->AddAcceptor(std::move(acceptor));
  }

Why the inline constructor. This feels like is a super expensive operation.


class TurtleServer {
 public:
  auto GetPool() -> ThreadPool * { return pool_.get(); }
  auto GetLooper() -> Looper * { return looper_.get(); }
};

You have many accessor functions like there. I ask my self does client code really need to look this much into the guts of these classes? Consider if you can remove implementation details from the API.


class EchoServer : public TurtleServer {
 public:
  explicit EchoServer(NetAddress server_address)
      : TurtleServer(server_address) {}

  void OnAccept(Connection *server_conn) final {}

  void OnHandle(Connection *client_conn) final {
    int from_fd = client_conn->GetFd();
    auto [read, exit] = client_conn->Recv();
    if (exit) {
      client_conn->GetLooper()->DeleteConnection(from_fd);
      // client_conn ptr is destoryed and invalid below here, do not touch it
      // again
      return;
    }
    if (read) {
      client_conn->WriteToWriteBuffer(client_conn->ReadAsString());
      client_conn->Send();
      client_conn->ClearReadBuffer();
    }
  }
};

This feels very old school OO. Consider providing an API with callbacks.

Consider the following:

int main() {
  auto server = turtle::Server(turtle::Address("0.0.0.0", 20080));
  server.onHandle([&] (turtle::Connection& conn) {
    auto [read, exit] = conn.Recv();
    if (exit) {
      server.Close(conn);
      return;
    }
    if (read) {
      conn.Send(conn.ReadAsString());
    }  
  });
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much rioki for going through the details and proper practices with me. Really appreciate your time and effort. I will think and go over these improvement suggestion and revise accordingly! \$\endgroup\$ Dec 30, 2022 at 21:44
2
\$\begingroup\$

General Observations

First off, nice a Server and Client Library in 1250 lines of code!

This review does comment on some code in the repository that is not included in the question.

One of the rules of code review that you should become familiar with is that we want to see the code as written, that means that rather than show us the helper classes the way you are we want to see the actual header files and source files for example util.h, buffer.h, buffer.cpp, etc.

It isn't clear why you want so many separate libraries. There is a need for libraries so that the server and the client can share the code but why 1 library for each major object rather than 1 total library? This is even more true when the header files have dependencies on each other (looper.h includes acceptor.h, connection.h, poller.h and thread_pool.h and acceptor.h includes connection.h, looper.h, and net_address.h).

Sometime we do design reviews as part of code review, but if you are looking primarily for a design review, Software Engineering might be a better community, just make sure to follow their guidelines first. Some things I would like to see from the design point of view would be UML diagrams, specifically class diagrams and sequence diagrams. One of the things I find concerning about the code is the way the classes depend on each other, the design does not seem to be modular enough. The classes need to be partitioned better.

When you are using git to manage the source make sure that you are pointing to the correct branch before you update the repository. Your latest edit was to the Code Review Branch rather than main.

Portability

Rather than depending on a portable compiler, such as Clang, stick to the C++ standard as much as possible and use conditional compilation when necessary. Cmake is portable, you can use cmake to do most of the portability.

Different compilers provide different levels of support for C++ standards, don't depend on just one.

This project might be more interesting if it supported portability to other systems without requiring Docker. The dependency on Linux / Unix headers could be in conditional compilation code.

Let the Compiler do the Error Checking

Rather than depending on a separate lint program, use all the errors and warnings that your compiler can provide. Both [Clang and gcc support the following switches(https://caiorss.github.io/C-Cpp-Notes/compiler-flags-options.html) which will catch the problems during build time rather than during a lint session:

  • -Wall - Provide all warning messages
  • -Wextra - Enables extra flags not enabled by -Wall, such as -Wsign-compare (C only), -Wtype-limits, -Wuninitialized
  • -pedantic - Issue all warning required by ISO C and ISO C++ standard, it issues warning whenever there are compiler extensions non compliant to ISO C or C++ standard.
  • -Werror - Treat all warnings as errors.

Suggested Modifications to CMakeLists.txt

I Added these options to the CMakeLists.txt file in my copy of your repository I am building using g++12 (gcc) rather than Clang.

TARGET_COMPILE_OPTIONS(echo_server PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(echo_client PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(buffer PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(net_address PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(poller PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(socket PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(connection PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(looper PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(thread_pool PRIVATE -Wall -Wextra -pedantic -Werror)
TARGET_COMPILE_OPTIONS(acceptor PRIVATE -Wall -Wextra -pedantic -Werror)

Some Error Messages Generated by the Above Build:

This was built on Ubuntu 22.04 using GNU C++ version 12 (Ubuntu currently downloads with version 11). I was able to get rid of most of these problems by using Forward Declarations and removing unnecessary header files.

[ 55%] Built target poller
In file included from /home/user1/Documents/CodeReview/Turtle/src/looper.cpp:12:
/home/user1/Documents/CodeReview/Turtle/src/include/looper.h: In constructor ‘TURTLE_SERVER::Looper::Looper(TURTLE_SERVER::ThreadPool*)’:
/home/user1/Documents/CodeReview/Turtle/src/include/looper.h:61:27: error: ‘TURTLE_SERVER::Looper::poller_’ will be initialized after [-Werror=reorder]
   61 |   std::unique_ptr<Poller> poller_;
      |                           ^~~~~~~
/home/user1/Documents/CodeReview/Turtle/src/include/looper.h:60:15: error:   ‘TURTLE_SERVER::ThreadPool* TURTLE_SERVER::Looper::pool_’ [-Werror=reorder]
   60 |   ThreadPool *pool_;
      |               ^~~~~
/home/user1/Documents/CodeReview/Turtle/src/looper.cpp:16:1: error:   when initialized here [-Werror=reorder]
   16 | Looper::Looper(ThreadPool *pool)
      | ^~~~~~
[ 65%] Built target socket
In file included from /home/user1/Documents/CodeReview/Turtle/src/connection.cpp:12:
/home/user1/Documents/CodeReview/Turtle/src/include/connection.h: In constructor ‘TURTLE_SERVER::Connection::Connection(TURTLE_SERVER::Looper*, std::unique_ptr<TURTLE_SERVER::Socket>)’:
/home/user1/Documents/CodeReview/Turtle/src/include/connection.h:83:11: error: ‘TURTLE_SERVER::Connection::looper_’ will be initialized after [-Werror=reorder]
   83 |   Looper *looper_;
      |           ^~~~~~~
/home/user1/Documents/CodeReview/Turtle/src/include/connection.h:82:27: error:   ‘std::unique_ptr<TURTLE_SERVER::Socket> TURTLE_SERVER::Connection::socket_’ [-Werror=reorder]
   82 |   std::unique_ptr<Socket> socket_;
      |                           ^~~~~~~
/home/user1/Documents/CodeReview/Turtle/src/connection.cpp:16:1: error:   when initialized here [-Werror=reorder]
   16 | Connection::Connection(Looper *looper, std::unique_ptr<Socket> socket)
      | ^~~~~~~~~~
/home/user1/Documents/CodeReview/Turtle/src/include/connection.h:82:27: error: ‘TURTLE_SERVER::Connection::socket_’ will be initialized after [-Werror=reorder]
   82 |   std::unique_ptr<Socket> socket_;
      |                           ^~~~~~~
/home/user1/Documents/CodeReview/Turtle/src/include/connection.h:80:27: error:   ‘std::unique_ptr<TURTLE_SERVER::Buffer> TURTLE_SERVER::Connection::read_buffer_’ [-Werror=reorder]
   80 |   std::unique_ptr<Buffer> read_buffer_;
      |                           ^~~~~~~~~~~~
/home/user1/Documents/CodeReview/Turtle/src/connection.cpp:16:1: error:   when initialized here [-Werror=reorder]
   16 | Connection::Connection(Looper *looper, std::unique_ptr<Socket> socket)
      | ^~~~~~~~~~

Code Review

The use of only one or two spaces as indentation makes the code harder to read, I prefer 4 spaces myself, some programmers prefer 8 spaces. In some cases the use of only one or two spaces can lead to very complex code since the level of indentation can be an indicator on when to create a function. The readability problem becomes clearer in the .cpp files than in the source files.

Symbolic Constants in C++

C++ certainly accepts the old C Style macros, but for symbolic constants such as TIMEOUT in looper.h and INITIAL_BUFFER_CAPACITY in buffer.h it might be better to use constexpr since old C style macros are not type safe.

static constexpr size_t INITIAL_BUFFER_CAPACITY = 1024;
static constexpr int TIMEOUT = 3000;

Note: These 2 constants should be defined within the TURTLE_SERVER namespace.

The use of macros in include guards is fine, I personally prefer it over #pragma once.

Indentation in Namespaces

Classes and other namespaces should be indented inside a namespace.

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code multiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

In the file util.h the macro declaration NON_COPYABLE_AND_MOVEABLE can be defined on terms of the previous 2 macros:

#define NON_COPYABLE(class_name)           \
  class_name(const class_name &) = delete; \
  class_name &operator=(const class_name &) = delete

#define NON_MOVEABLE(class_name)      \
  class_name(class_name &&) = delete; \
  class_name &operator=(class_name &&) = delete

#define NON_COPYABLE_AND_MOVEABLE(class_name)   \
  NON_COPYABLE(class_name)          \
  NON_MOVEABLE(class_name)

This reduces the amount of debugging and maintenance necessary.

The use of Templates is the primary replacement for C style macros in C++.

Only Include What is Necessary to Compile

In the header file looper.h it ins't clear that the include for acceptor.h is necessary, since the code contains:

class Acceptor;
/**
 * This Looper acts as the central coordinator between executor (ThreadPool) and
 * event polling (Poller)
 * */
class Looper {

Use more forward references to decrease the need for including header file unnecessarily.

To keep compile/build times down it is better to only include what is needed to compile. The method the C and C++ preprocessors use to include files is to replace the #include statement with the code from the source file, this can lead to huge files that take some time to compile even in modern compilers.

A second reason to limit what you include in header files is that the code in the .cpp is more understandable if the include files are listed. The include for acceptor.h is necessary in looper.cpp, but not in looper.h.

There is no reason for acceptor.h to include poller.h just to access the system header file epoll.h.

\$\endgroup\$
1
  • \$\begingroup\$ Hi pacmaninbw! Thanks so much for your insights on the building part of the project! Indeed you clarify one of my lasting confusion about when to forward declaration and when to include header files, when two classes interact with each other! I will take notes of the advice here and revise upon them! 🙇 \$\endgroup\$ Dec 30, 2022 at 21:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.