Mryntu

I'm working on a project (studying) and I have a lot of time for proof of concepts and write something from scratch.

Basically I'm creating a Http Server (simple, but not too simple) in C++ using sockets and multi-threading.

There are two topics that I'm concerned about: the design pattern of my code structure and the efficiency of my thread implementation. Obs: a little worried about C++ best practices, since I'm diving too fast into C++ (Am I abusing of std items?, since this require a low-level implementation).

Server is the main file, that calls Routes, Request and Response. Request and Response contains Struct (that contains Status Code). And Routes contains Request and Response.

IMPORTANT: I'm using nlohmann/json (https://github.com/nlohmann/json) and j-ulrich status-codes (https://github.com/j-ulrich/http-status-codes-cpp).

Server (Accepting Sockets and Multi-threading):

#pragma once

#include <unistd.h>
#include <stdio.h>
#include <sys/socket.h>
#include <stdlib.h>
#include <netinet/in.h>
#include <string.h>
#include <iostream>
#include <unordered_map>
#include <thread>
#include <mutex>
#include <condition_variable>

#include "Server/Request.h"
#include "Server/Response.h"
#include "Server/Struct.h"
#include "Server/Routes.h"
#include "Tools/Logger.h"

class Server 
 public:
 Server(unsigned int port, unsigned int max_connections = 64, unsigned int thread_count = 5);
 ~Server();

 bool setRoute(std::string path, Struct::Methods method, void (*callback)(Request*, Response*));
 bool doListen();
 bool doStop();

 private:
 unsigned int _port;
 unsigned int _max_connections;
 unsigned int _thread_count;

 std::mutex _mutex;
 std::condition_variable _condition;

 bool _signal;

 std::vector<unsigned int> _queue;

 std::thread* _thread_consume;
 std::thread* _thread_process;

 Routes* _routes;

 int _socket;
 struct sockaddr_in _address;
 bool _listen;

 bool _doStop();
 bool _doCreateSocket(int& socket_in);
 bool _doBindSocket(int file_descriptor);
 void _doConsumeSocket();
 void _doProcessSocket(int id);
 bool _doProcessRequest(Request* request, Response* response);
;

#include "Server/Server.h"

Server::Server(unsigned int port, unsigned int max_connections, unsigned int thread_count) 
 if (port > 65535) 
 Logger::doSendMessage(Logger::TYPES::ERROR, "[Port must be something between 0 and 65535 on Server::Constructor.");
 

 if (max_connections < 1) 
 Logger::doSendMessage(Logger::TYPES::ERROR, "Max connections can't be lower than 1 on Server::Constructor.");
 

 _port = port;
 _max_connections = max_connections;
 _thread_count = thread_count;

 _routes = new Routes();

 int status = _doCreateSocket(_socket);
 if (!status) 
 Logger::doSendMessage(Logger::TYPES::ERROR, "Failed to create socket on Server::Constructor.");
 

 if (!_doBindSocket(_socket)) 
 Logger::doSendMessage(Logger::TYPES::ERROR, "Failed to bind socket on Server::Constructor.");
 ;

 _signal = false;
 _listen = false;


Server::~Server() 
 _doStop();

 shutdown(_socket, SHUT_RD);
 close(_socket);

 try 
 _thread_consume->join();
 for (size_t i = 0; i < _thread_count; i++) 
 _thread_process[i].join();
 
 catch (...) 

 delete _thread_consume;
 delete[] _thread_process;
 delete _routes;


bool Server::setRoute(std::string path, Struct::Methods method, void (*callback)(Request*, Response*)) 
 return _routes->setRoute(path, method, callback);


bool Server::doListen() 
 if (_listen) return false;

 int status;

 status = listen(_socket, _max_connections);
 if (status < 0) return false;

 Logger::doSendMessage(Logger::TYPES::INFO, "Server running with success at port " + std::to_string(_port) + ".");

 _listen = true;

 _thread_consume = new std::thread(&Server::_doConsumeSocket, this);
 _thread_process = new std::thread[_thread_count];
 for (size_t i = 0; i < _thread_count; i++) 
 _thread_process[i] = std::thread(&Server::_doProcessSocket, this, i);
 

 return true;


bool Server::doStop() 
 return _doStop();


bool Server::_doStop() 
 if (!_listen) return false;
 
 std::lock_guard<std::mutex> lock(_mutex);
 _listen = false;
 
 _condition.notify_one();
 return true;


bool Server::_doCreateSocket(int& socket_in) SO_REUSEPORT, &opt, sizeof(opt));
 if (error) return false;

 socket_in = file_descriptor;
 return true;


bool Server::_doBindSocket(int file_descriptor) 
 if (!file_descriptor) return false;

 _address.sin_family = AF_INET;
 _address.sin_addr.s_addr = INADDR_ANY;
 _address.sin_port = htons(_port);

 int status;

 status = bind(file_descriptor, (struct sockaddr*) &_address, sizeof(_address));
 if (status < 0) return false;

 return true;


void Server::_doConsumeSocket() 
 int socket_in;
 int address_size = sizeof(_address);

 while (_listen) 
 socket_in = accept(_socket, (struct sockaddr*) &_address, (socklen_t*) &address_size);
 if (socket_in < 0) continue;

 
 std::lock_guard<std::mutex> lock(_mutex);
 _queue.push_back(socket_in);
 _signal = true;
 

 _condition.notify_one();
 


void Server::_doProcessSocket(int id) 
 while (_listen) 
 int queue_size = 0;

 
 std::unique_lock<std::mutex> lock(_mutex);
 _condition.wait(lock,
 [this] 
 if (this->_signal) return true;
 if (!this->_listen && !this->_queue.size()) return true;
 return false;
 
 );
 queue_size = _queue.size();
 

 if (!queue_size) 
 
 std::lock_guard<std::mutex> lock(_mutex);
 _signal = false;
 

 _condition.notify_one();
 continue;
 

 int socket_in = 0;
 
 std::lock_guard<std::mutex> lock(_mutex);
 socket_in = _queue[0];
 _queue.erase(_queue.begin());
 

 Request* request = new Request(socket_in);
 Response* response = new Response(socket_in);

 int status = _doProcessRequest(request, response);

 delete request;
 delete response;
 close(socket_in);
 


bool Server::_doProcessRequest(Request* request, Response* response) 
 if (!request->isValid()) 
 response->doSendError(HttpStatus::Code::BadRequest, "Invalid request.");
 return false;
 

 std::string path = request->getPath();
 Struct::Methods method = request->getMethod();

 Routes::Route route;
 if (!(_routes->getRoute(path, method, route) && route.isValid())) 
 response->doSendError(HttpStatus::Code::Forbidden, "Path invalid/not found.");
 return false;
 

 if (route.method != method) 
 response->doSendError(HttpStatus::Code::MethodNotAllowed, "Method invalid/not found.");
 return false;
 

 void (*callback)(Request*, Response*) = route.callback;
 callback(request, response);

 if (!response->isSent()) 
 response->doSendError(HttpStatus::Code::ServiceUnavailable, "Resource was not found or can't respond now.");
 

 return true;

Request (parsing) and Response (sending)

Request

#pragma once

#include <unistd.h>
#include <string.h>
#include <iostream>
#include <string>
#include <vector>
#include <unordered_map>
#include <regex>
#include <json.hpp>

#include "Server/Struct.h"

using json = nlohmann::json;

class Request 
 public:
 Request(int socket, unsigned int buffer_size = 1024);
 ~Request();

 bool isValid();
 std::string getPath() return _attributes.path;
 Struct::Methods getMethod() return _attributes.method;
 std::unordered_map<std::string, std::string> getHeaders() return _attributes.headers;
 std::string getHeader(std::string header) return _attributes.headers[header];
 json getBody() return _attributes.body;

 private:
 int _socket;
 unsigned int _buffer_size;
 std::string _data;
 Struct::Attributes _attributes;
 bool _status;

 std::string _doReceiveData(int sock_in);
 bool _doParseData(std::string data, Struct::Attributes& attributes);
 std::vector<std::string> _doSplitText(std::string text, std::string delimiter);
 std::vector<std::string> _doSplitText(std::string text, std::string delimiter, int lock);
;

Request::Request(int socket, unsigned int buffer_size) 
 _socket = socket;
 _buffer_size = buffer_size;
 _status = false;

 _data = _doReceiveData(_socket);
 if (!_data.length()) return;

 bool result;
 result = _doParseData(_data, _attributes);

 if (!result) return;
 if (!_attributes.isValidRequest()) return;

 _status = true;


Request::~Request() 



bool Request::isValid() 
 return _status;


std::string Request::_doReceiveData(int sock_in) 
 char* buffer = new char[_buffer_size];
 memset(buffer, '', _buffer_size);
 read(sock_in, buffer, _buffer_size);

 std::string data;
 data.assign(buffer);
 delete[] buffer;
 return data;


bool Request::_doParseData(std::string data, Struct::Attributes& attributes) 
 std::string delimiter = "rn";
 std::vector<std::string> rows = _doSplitText(data, delimiter);
 if (!rows.size()) return false;

 std::string header = rows[0];
 rows.erase(rows.begin());

 if (!header.length()) return false;

 std::vector<std::string> parsed_header = _doSplitText(header, std::string(" "));
 if (parsed_header.size() < 2) return false;

 Struct::Methods method = Struct::doParseHttpMethod(parsed_header[0]);
 if (method == Struct::Methods::NONE) return false;

 std::string path = parsed_header[1];

 std::unordered_map<std::string, std::string> headers;
 for (size_t i = 0; i < rows.size(); i++) 
 std::string row = rows[i];
 delimiter = ":";

 std::vector<std::string> splited = _doSplitText(row, delimiter, true);
 if (splited.size() != 2) continue;

 headers[splited[0]] = splited[1];
 

 _attributes.method = method;
 _attributes.path = path;
 _attributes.headers = headers;

 std::string content_length = headers["Content-Length"];
 int content_size = 0;

 if (content_size = atoi(content_length.c_str())) 
 std::string body = data.substr(data.length() - content_size, data.length());
 json parsed_body = json::parse(body, nullptr, false);
 if (parsed_body != NULL && !parsed_body.is_discarded()) _attributes.body = parsed_body;
 

 return true;


std::vector<std::string> Request::_doSplitText(std::string text, std::string delimiter) 
 std::vector<std::string> result;
 int delimiter_length = delimiter.length();

 std::string block;
 std::string region;
 int index = 0;

 for (size_t i = 0; i < text.length(); i++) 
 block = text.substr(i, delimiter_length);
 if (block.length() != delimiter_length) continue;

 if (block == delimiter) 
 region = text.substr(index, i - index);
 result.push_back(region);
 index = i + delimiter_length;
 
 

 return result;


std::vector<std::string> Request::_doSplitText(std::string text, std::string delimiter, int lock) 
 ...

Response

#pragma once

#include <unistd.h>
#include <iostream>
#include <string>
#include <unordered_map>

#include "Server/Struct.h"

class Response 
 public:
 Response(int socket_in);
 ~Response();

 bool isSent() return _sent;
 void setCode(HttpStatus::Code code) _attributes.code = code;
 void setHeader(std::string key, std::string value) _attributes.headers[key] = value;
 void setBody(json body) _attributes.body = body;
 void doClearHeaders() _attributes.headers.clear();
 void doClearBody() _attributes.body = json::value_t::object;

 bool doSendSuccess();
 bool doSendError(HttpStatus::Code code, const std::string& message);

 private:
 int _socket;
 bool _sent;

 Struct::Attributes _attributes;

 bool _doSendPayload();
 bool _doCreatePayload(std::string& payload);
;

#include "Response.h"

Response::Response(int socket_in) 
 _socket = socket_in;
 _sent = false;


Response::~Response() 



bool Response::doSendSuccess() 
 setCode(HttpStatus::Code::OK);
 setHeader("Connection", "Closed");
 return _doSendPayload();


bool Response::doSendError(HttpStatus::Code code, const std::string& message) 
 setCode(code);
 doClearHeaders();
 doClearBody();

 setHeader("Connection", "Closed");

 json body;
 body["error"] = ;
 body["error"]["code"] = code;
 body["error"]["message"] = message;

 setBody(body);
 return _doSendPayload();


bool Response::_doSendPayload() 
 if (_sent) return false;

 int status;

 setHeader("Server", "Dark");
 setHeader("Content-Type", "application/json");

 std::string payload;
 status = _doCreatePayload(payload);
 if (!status) return false;

 status = write(_socket, payload.c_str(), payload.size());
 if (status < 1) return false;

 _sent = true;
 return true;


bool Response::_doCreatePayload(std::string& payload) 
 std::string current_payload;
 std::string data = _attributes.body.dump(4);

 int data_length = data.size();

 if (data_length) 
 _attributes.headers["Content-Length"] = std::to_string(data_length);
 

 current_payload += _attributes.version + " " + std::to_string((int) _attributes.code) + " " + HttpStatus::getReasonPhrase(_attributes.code) + "rn";

 std::unordered_map<std::string, std::string>::iterator iterator;
 for (iterator = _attributes.headers.begin(); iterator != _attributes.headers.end(); iterator++)
 std::string key = iterator->first;
 std::string value = iterator->second;

 current_payload += key + ": " + value + "rn";
 

 if (data_length) current_payload += "rn" + data + "rnrn";

 payload = current_payload;
 return true;

Routes

#pragma once

#include <iostream>
#include <string>
#include <vector>

#include "Server/Request.h"
#include "Server/Response.h"

class Routes 
 public:
 Routes();
 ~Routes();

 struct Route 
 std::string path;
 Struct::Methods method;
 void (*callback)(Request*, Response*);

 bool isValid() 
 ;

 bool setRoute(std::string path, Struct::Methods method, void (*callback)(Request*, Response*));
 bool getRoute(std::string path, Struct::Methods method, Route& route);

 private:
 std::vector<Route> _routes;

 int _getRouteIndex(std::string path, Struct::Methods method);
;

#include "Routes.h"

Routes::Routes() 



Routes::~Routes() 



bool Routes::setRoute(std::string path, Struct::Methods method, void (*callback)(Request*, Response*)) 
 if (path.length() < 1) return false;
 if (_getRouteIndex(path, method) >= 0) return false;
 if (method < Struct::Methods::FIRST 

bool Routes::getRoute(std::string path, Struct::Methods method, Routes::Route& route) 
 int index = _getRouteIndex(path, method);
 if (index < 0) return false;
 route = _routes[index];
 return true;


int Routes::_getRouteIndex(std::string path, Struct::Methods method) 
 for (size_t i = 0; i < _routes.size(); i++) 
 Route* route = &_routes[i];
 if (route->path == path && route->method == method) 
 return i;
 
 
 return -1;

Struct and StatusCode

StatusCode

https://github.com/j-ulrich/http-status-codes-cpp

Struct

#pragma once

#include <json.hpp>

#include "Server/StatusCode.h"

using json = nlohmann::json;

class Struct 
 public:
 enum class Methods 
 NONE = 0, GET = 1, POST = 2, FIRST = GET, LAST = POST
 ;

 struct Attributes 
 const std::string version = "HTTP/1.1";

 std::string path;
 Methods method;
 HttpStatus::Code code;
 std::unordered_map<std::string, std::string> headers;
 json body;

 Attributes() 
 code = HttpStatus::Code::InternalServerError;
 body = json::value_t::object;
 

 bool isValidRequest() 
 if (!path.length()) return false;
 if (method < Methods::FIRST 

 bool isValidResponse() 
 if (!headers.size()) return false;
 return true; 
 
 ;

 static Methods doParseHttpMethod(std::string value) 
 Methods target = Methods::NONE;

 if (value == "GET") target = Methods::GET;
 if (value == "POST") target = Methods::POST;

 return target;
 

 private:
;

Main (usage):

#include "Server/Server.h"

void exec(Request* request, Response* response) 
 json body;

 body["foo"] = 123;
 body["bar"] = true;

 response->setBody(body);
 response->doSendSuccess();


int main(int argc, char* argv[]) 
 Server* server = new Server(5000);
 server->setRoute("/getStatus", Struct::Methods::GET, exec);
 server->setRoute("/postStatus", Struct::Methods::POST, exec);
 server->doListen();

 // let threads live for some time before they eternaly be gone
 /*
 actually I'm stuck with this sleep, I don't know how to hold 
 this until caller call doStop() without using while and
 consuming process power
 */
 sleep(30);

 delete server;
 return 1;

Basically I'm mirroring NodeJS express API Usage:
server.setRoute(path, route, callback);

So, what can be done to improve my code in terms of optimization and efficiency?

Thanks in advance.

Note: This review focuses on the use of C++, rather than the functionality.

Naming:

• IMHO, the use of "do" at the start of function names is unnecessary and makes the code harder to read. The names would be fine without it (e.g. sendSuccess, sendError, createSocket all make perfect sense).

Server:

• If the port must always fit in a 16 bit unsigned int, we can use std::uint16_t (from the <cstdint> header) instead of an unsigned int.




• The new keyword should almost never be used in modern C++. If we need to create it on the heap, _routes should be a std::unique_ptr (from <memory>), which will be cleaned up automatically for us. In this case it looks like the variable could just be created on the stack (i.e. declared as Routes _routes;).




• _doCreateSocket() returns a bool, but the Server constructor uses an int to hold the return type.



• 
  

  It's better to use the constructor's member initializer list to initialize variables where possible (it's neater, and we don't have to worry about initializing objects twice), e.g.:

  
  
  

  Server::Server(std::uint16_t port, unsigned int max_connections, unsigned int thread_count):
   _port(port),
   _max_connections(max_connections),
   _thread_count(thread_count),
   _signal(false),
   _thread_consume(nullptr),
   _thread_process(nullptr),
   _routes(nullptr),
   _socket(-1), // or something
   _listen(false)
  
   if (max_connections < 1) 
   Logger::doSendMessage(Logger::TYPES::ERROR, "Max connections can't be lower than 1 on Server::Constructor.");
   
  
   if (!_doCreateSocket(_socket)) 
   Logger::doSendMessage(Logger::TYPES::ERROR, "Failed to create socket on Server::Constructor.");
   
  
   if (!_doBindSocket(_socket)) 
   Logger::doSendMessage(Logger::TYPES::ERROR, "Failed to bind socket on Server::Constructor.");
   
  
  

  
  



• Note that plain data variables (e.g. pointers, ints) are left uninitialized (and may contain any random value) unless we explicitly initialize them. It's safest to always set them to a known value in the constructor.




• _thread_consume can also be created on the stack (the std::thread default constructor doesn't launch a new thread), and _thread_process can be a std::vector<std::thread>. This saves us from having to do any manual memory management.




• Prefer to use std::function from the <functional> header, instead of raw function pointers. (e.g. std::function<void(Request*, Response*)>).




• The request and response variables in Server::_doProcessSocket should be created on the stack. We can still pass them by pointers if necessary by taking their addresses (_doProcessRequest(&request, &response)), or (better) we could pass them by reference.




• The status code returned by Server::_doProcessRequest() isn't used.




• In Server::_doProcessRequest(), the if (route.method != method) check is unnecessary, since we used the method while finding the route.




• The forwarding from doStop to _doStop is unnecessary.




• This class is doing several things. It manages a thread pool, as well as doing raw socket stuff. We could definitely split the socket functionality into a separate class.

Request:

• Member functions that don't alter the member variables of a class should be declared const, e.g.: bool isValid() const;. This means we can make proper use of const and const& variables, allowing the compiler to perform better optimisations, and preventing programmer error.




• The getter functions in this class all return by value. This probably results in some unnecessary copies being made, which may be expensive where the objects require allocation of memory (e.g. copying the unordered_map / strings). It might be better to return by const& instead, e.g.: std::unordered_map<std::string, std::string> const& getHeaders();. This still prevents the caller from altering referenced variable, but allows them to decide whether to copy it, copy part of it, or not copy it at all.




• In Request::_doReceiveData, we can use a std::vector<char> for the buffer rather than doing manual memory management. (It's guaranteed to provide contiguous memory, which we can access using the .data() member function).

Routes:

• Routes::setRoute should probably use the Route::isValid method, rather than duplicating the checks.




• There's some unnecessary string copies in getRoute. We should pass the variable as a reference: const std::string& path instead.



• 
  

  Using iterators and the standard library search algorithms is more idiomatic C++ than indices. e.g.:

  
  
  

  auto route = std::find_if(_routes.begin(), routes.end(), 
   [] (Route const& route) return (route.path == path) && (route->method == method); );
  
  if (route == routes.end()) // route not found!
  

  
  



• (Unless it's reused elsewhere, I'd be inclined to remove the Routes class in favor of a std::vector<Route> _routes; in the Server class.)

Main:

• (Use std::this_thread::sleep_for for a portable sleep function.)




• I think std::condition_variable::wait() may be what you're looking for.