HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple Multi-Threaded Web Server

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplemultiwebserverthreaded

Problem

Need a way to test a web crawler.

Threw together this web server that will replay previously saved pages from a real web server.
Headers

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#include 
#include 
#include 
#include 

#include 
#include 
#include 

extern "C" void* workerThread(void*);


Multi Threading Stuff

```
class SimpleThread
{
pthread_t thread;

SimpleThread(SimpleThread const&);
SimpleThread& operator=(SimpleThread const&);

public:
SimpleThread()
{
if (::pthread_create(&thread, NULL, workerThread, NULL) != 0)
{
throw std::runtime_error("Failed to Start Thread");
}
}
~SimpleThread()
{
void* result;
if (::pthread_join(thread, &result) != 0)
{
throw std::runtime_error("Failed to Join Thread");
}
}
};

class SimpleCondition;
class SimpleMutex
{
SimpleMutex(SimpleMutex const&);
SimpleMutex operator=(SimpleMutex const&);

friend class SimpleCondition;
pthread_mutex_t mutex;

public:
SimpleMutex()
{
if (::pthread_mutex_init(&mutex, NULL) != 0)
{
throw std::runtime_error("Failed to Create Mutex");
}
}
~SimpleMutex()
{
::pthread_mutex_destroy(&mutex);
}
void lock()
{
if (::pthread_mutex_lock(&mutex) != 0)
{
throw std::runtime_error("Filed to lock mutex");
}
}
void unlock()
{
if (::pthread_mutex_unlock(&mutex) != 0)
{
throw std::runtime_error("Faile to unlock mutex");
}
}
};
class Locker
{
SimpleMutex& m;
public:
Locker(SimpleMutex& m)
: m(m)
{
m.lock();
}
~Locker()
{

Solution

First of all, I really liked your C++ code and even though I am not a professional I think there are some points to consider.

To the future!

It is 2016 so I would say we can safely use C++11 today (at least if you are not held back by embedded, an ancient compiler, etc.). There being at least three major compilers with C++11 support this shoud not be a problem.

I went through your code from top to bottom and the first thing was SimpleThread. It is good but we do not need it because there is std::thread

int main() {
  std::vector workers{std::thread::hardware_concurrency()};
  ...
  for(auto& worker : workers) {
    worker = std::thread(workerThread, nullptr);
  }
}


So instead of using 8 as a safe assumption for our threads I used hardware_concurrency() to ask the system how many cores it has/we can utilize. The constructor of threadis more generic than SimpleThread and takes the function as a parameter. thread being templated we can safely use a C++ function with arbitrary arguments which allows us to change workerThread to

void workerThread(WorkQueue& work)


and enables us to remove the global variable and the unneccessary return null which would be in C++11 terms return nullptr. Because we pass a reference we have to use thread(workerThread, std::ref(work)) to allow the template magic to find the right workerThread.

Going forward, your classes SimpleCondition and SimpleMutex are redundant as well because there are std::condition_variable and std::mutex which do pretty much the same thing.

Instead of releaseAll() and notify we use notify_all() and notify_one() (which describes their use cases and similarity better). To lock the mutex we use std::unique_lock lock{access} which works like your Lock. A real difference is in condition.wait() because it takes the lock and not the mutex as a parameter condition.wait(lock).

I know everyone (in C/C++) likes to implement their linked lists, but why bother when there is no real benefit. It takes more work, it increases cache misses because the objects are scattered in the memory and in your implementation it locks Job to this implementation without a reason. Job should just be a data structure regardless of the container it is put in. So I removed Job* next from it.

To accomodate WorkQueue has to change as well. I decided to put a std::queue in it because it is exactly what we need.

class WorkQueue {
  std::queue> jobs;
  std::mutex access;
  std::condition_variable condition;
  bool finished;

  public:
    WorkQueue()
      : finished(false)
    {}
    ~WorkQueue() {
      finished = true;
      condition.notify_all();
    }
    void addItem(std::unique_ptr another) {
      {
        std::unique_lock lock{access};
        jobs.push(std::move(another));
      }
      condition.notify_one();
    }
    std::unique_ptr getItem() {
      std::unique_lock lock{access};
      condition.wait(lock, [this] () { return !jobs.empty() || finished; });
      if (finished) {
        return std::unique_ptr(nullptr);
      }
      std::unique_ptr nextJob = std::move(jobs.front());
      jobs.pop();
      return nextJob;
    }
};


While I was at it I changed Job* to a std::unique_ptr. I saw you used std::auto_ptr, do not use it, it is deprecated and will be removed in C++17. unique_ptr is not copy-constructible/-assignable therefore we have to move it into jobs in addItem and out of it in getItem.

A more drastic change is the wait logic. I could have used condition.wait(lock) in a while loop but there is an overload of wait() which takes a predicate to check if waiting is over and it fits this perfectly.

In your getJob I created the unique_ptr using std::make_uniqe() which is the highly recommended way to create unique_ptrs. While I was at it I made it static because it removes the need to instantiate Pages.

Destroy all the things!

Wait what?! Even though I wanted to show you all these neat ways which simplify your code in the end I deleted all your thread code and most of my changes because there is a simpler solution to your problem.

You know that IO takes time and blocks so you thought of a way to make it asynchronous. The common approach, as you did, is to implement some kind of threadpool logic with workers and creators. But there is an alternative, std::async. It encapsulates the asynchronous logic and is required to act like if it was a std::thread. Most implementations problably use a threadpool so it is exactly what you did, just less work for you.

I removed SimpleQueue and changed workerThread (not being a thread anymore)

```
void handleJob(std::unique_ptr job) {
if (job->sockfd) {
writeFile(job->sockfd, job->fileName + ".head");
writeFile(job->sockfd, job->fileName + ".body");
}
}

int main() {
std::vector> pending_futures;
SimpleSocket socket(80);

int connection;
while(connection = socket.waitForConnection()) {
std::string page = get

Code Snippets

int main() {
  std::vector<std::thread> workers{std::thread::hardware_concurrency()};
  ...
  for(auto& worker : workers) {
    worker = std::thread(workerThread, nullptr);
  }
}
void workerThread(WorkQueue& work)
class WorkQueue {
  std::queue<std::unique_ptr<Job>> jobs;
  std::mutex access;
  std::condition_variable condition;
  bool finished;

  public:
    WorkQueue()
      : finished(false)
    {}
    ~WorkQueue() {
      finished = true;
      condition.notify_all();
    }
    void addItem(std::unique_ptr<Job> another) {
      {
        std::unique_lock<std::mutex> lock{access};
        jobs.push(std::move(another));
      }
      condition.notify_one();
    }
    std::unique_ptr<Job> getItem() {
      std::unique_lock<std::mutex> lock{access};
      condition.wait(lock, [this] () { return !jobs.empty() || finished; });
      if (finished) {
        return std::unique_ptr<Job>(nullptr);
      }
      std::unique_ptr<Job> nextJob = std::move(jobs.front());
      jobs.pop();
      return nextJob;
    }
};
void handleJob(std::unique_ptr<Job> job) {
  if (job->sockfd) {
    writeFile(job->sockfd, job->fileName + ".head");
    writeFile(job->sockfd, job->fileName + ".body");
  }
}

int main() {
  std::vector<std::future<void>> pending_futures;
  SimpleSocket socket(80);

  int connection;
  while(connection = socket.waitForConnection()) {
    std::string page = getPageFromRequest(connection);
    pending_futures.push_back(std::async(std::launch::async, handleJob, Pages::getJob(connection, page)));
  }
}
namespace fs = std::experimental::filesystem;
...
bool ok = fs::is_regular_file(fileName + ".head") && fs::is_regular_file(fileName + ".body");

Context

StackExchange Code Review Q#122043, answer score: 6

Revisions (0)

No revisions yet.