Arsthbt

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() method > Struct::Methods::LAST) return false;
 if (callback == nullptr) return false;
 return true;
 
 ;

 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*)) 

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.

It's not bad code for someone new to C++. Here are some observations and suggestions that may help you improve your code.

Avoid relative paths in #includes

Generally it's better to omit relative path names from #include files and instead point the compiler to the appropriate location. So instead of this:

#include "Server/Routes.h"
#include "Tools/Logger.h"

write this:

#include "Routes.h"
#include "Logger.h"

For gcc, you'd use -I to tell the compiler where to find these files. This makes the code less dependent on the actual file structure, and leaving such details in a single location: a Makefile or compiler configuration file. With cmake, we can use include_directories.

Prefer to avoid using new and delete directly

The server variable within the main() function doesn't really need to be allocated via new. The same is true of the _routes member of Server and probably some other places as well. That way, it is automatically created with the correct length and then discarded when the function is complete or the owning object is deleted.

Avoid leading underscores in names

Anything with a leading underscore is a reserved name in C++ (and in C). See this question for details.

Prefer modern initializers for constructors

The constructor use the more modern initializer style rather than the old style you're currently using. Instead of this:

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

one could write this:

Response::Response(int socket_in) :
 _socketsocket_in,
 _sentfalse

Let the compiler create default destructor

The compiler will create a destructor by default which is essentially identical to what you've got in several places, so you can simply omit both the declaraton and implementation from your code.

Fix the bug

This is a very subtle bug, but a bug nonetheless. Within Server::setRoute() three threads are created, two of which take this as a parameter. The problem is that there's no guarantee that the object still exists for the duration of the launched threads. What you need is to use enable_shared_from_this with the class and then use a shared_ptr. This question explains a bit more. Generally speaking, it is somewhat difficult to write robust multithreaded code in C++ because there are many ways to create subtle bugs like this.

Avoid needless casts and variables

The current code contains these two lines:

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

But this could be much more simply written like this:

route.callback(request, response);

Use const where practical

The Request::isValid() and Routes::_getRouteIndex member functions do not alter the underlying objects and therefore should be declared const. That is, the declaration should be:

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

Use "range for" and simplify your code

The current Routes::_getRouteIndex looks like this:

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;

Using a range for it could be written like this:

int Routes::_getRouteIndex(std::string path, Struct::Methods method) const 
 int i0;
 for (const auto &r : _routes) 
 if (r.path == path && r.method == method) 
 return i;
 
 ++i;
 
 return -1;

However, even better in this instance, use the next suggestion.

Use library functions where appropriate

The code for Routes currently stores each Route in a std::vector and then goes through some machinations to recover the index. This could be done more simply by using a std::unordered_map. Also, it may be sufficient just to use the path as an index because the calling code checks that the method matches. Right now, it's not possible to trigger the "Method invalid/not found" error because getRoute only returns true if both the path and the method match.

Omit unused variables

Because argc and argv are unused, you could use the alternative form of main:

int main ()

There are also a number of other places in which passed variables are unused.