Minimal webserver in C++, second revision

First revision: Minimal webserver in C++

Here's my test question for C++ programmer job:

• Servers only static content, no cgi

• Single process, multithreaded

• 1000 concurrent request at least

• Valid http status codes and headers

• No external libraries, just STL, POSIX and glibc

Here's my code:

```
#include
#include
#include

#include
#include
#include
#include

#include
#include
#include
#include
#include

#include
#include
#include
#include

using namespace std;

template
class SafeQueue {
mutex m;
queue q;
condition_variable cv;
public:
SafeQueue() {}
SafeQueue(const SafeQueue &) = delete;

void push(T v) {
lock_guard lk(m);
q.push(move(v));
cv.notify_one();
}

bool try_pop(T &v) {
lock_guard lk(m);
if(q.empty()) {
return false;
}
v = move(q.front());
q.pop();
return true;
}
private:
/ data /
};

class Pool {
public:
typedef function Task;

Pool() : done(false) {
unsigned c = thread::hardware_concurrency();
//unsigned c = 1000;
printf("Pool() with %d threads\n", c);
for(unsigned i = 0; i sq;
vector vt;

void worker() {
while(!done) {
Task t;
if(sq.try_pop(t)) {
t();
} else {
this_thread::yield();
}
}
}
};

const char *response_200 = "HTTP/1.1 200 OK\nConnection: close\nContent-Type: text/html; charset=utf-8\n\nHello!";
const char *response_400 = "HTTP/1.1 400 Bad Request\nConnection: close\nContent-Type: text/html; charset=utf-8\n\nBad Request!";
const char *response_404 = "HTTP/1.1 404 Not Found\nConnection: close\nContent-Type: text/html; charset=utf-8\n\nNot Found!";

void handle_request(int cliefd)
{
ssize_t n;
char buffer[255];
const char *response;

string s, token;
istringstream ss(s);
vector token_list;

while (true)

Consistency in Copy

You have deleted the copy constructor of SafeQueue:

SafeQueue(const SafeQueue &) = delete;

But you have not done anything with the assignment operator (this seems inconsistency). Neither will work because the Mutex is non-copyable, but making it explicit is not a bad idea (it documents your intent). But if you do one, you should probably do the other.

Move not being used efficiently.

void push(T v) {
    lock_guard lk(m);
    q.push(move(v));
    cv.notify_one();
}

You move the value v into the queue. But you pass it by value to the method push() so a copy has already been made. I think it would be better to provide a complete move via push and a normal push by copy (rather than this half of each).

// A moving push
void push(T&& v) {
    lock_guard lk(m);
    q.push(std::forward(v));  // use std::forward to pass the
    cv.notify_one();             // parameter to anther call.
}                                // You use std::move() to initiate
                                 // The move operation.

// A copying push for non movable objects.
void push(T const& v) {          // Pass by const ref so you can pass temporary object
    lock_guard lk(m);
    q.push(v);
    cv.notify_one();
}

Wasted Condition Variable

You are not using your condition variable:

condition_variable cv;

The push notifies it when work is added. But the pop does not pause threads waiting for work to arrive. Looking forward to your Pool class you should definitely try and use the condition variable as the yield() method is not going to be very efficient (As the threads will keep being woken up and try to do work when there is none available).

Using Threads

Using threads is not the best way to handle lots of concurrent connections. Look up the 10K problem. Your get a much better throughput using a smaller number of threads and select/poll/epoll.

Your current technique will limit your throughput as each thread will be limited by the network throughput of the current connection. Some clients may read slowly and thus cause a denial of service attack on your server by just reading slowly from your connection.

If you use the select technique you are less likely to be limited by slow connections as a slow client will just cause you to move to another connection that can currently receive input and you can continue writing to alternative sockets.

Declare object close to the point you are going to use them

string s, token;
istringstream ss(s);
vector token_list;

These objects are declared at the top of the function handle_request but not used till after a loop (which may return). But you have paid for their construction cost even if you are not going to use them.

Also by declaring them close to the point of usage you can more easily see their types.

Not all read errors are bad

n = read(cliefd, buffer, sizeof(buffer) - 1);
    if(n == -1) {
        perror("read()");
        return;

Check the error code to see what the actual error is. There are some that are easily recoverable from. In fact you should recover from them otherwise you may lose a lot of connections.

n = read(cliefd, buffer, sizeof(buffer) - 1);
    if(n == -1) {
        if (errno == EINTR) {
             continue;
        }
        perror("read()");
        return;

String can append a buffer

buffer[n] = 0;
    s += string(buffer);

Much easier to write:

s.append(buffer, n);

StackExchange Code Review Q#84666, answer score: 6