current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

First revision: Minimal webserver in C++

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

  1. Servers only static content, no cgi
  2. Single process, multithreaded
  3. 1000 concurrent request at least
  4. Valid http status codes and headers
  5. No external libraries, just STL, POSIX and glibc

Here's my code:

#include <cstdio>
#include <cstdlib>
#include <cstring>

#include <sstream>
#include <vector>
#include <string>
#include <queue>

#include <thread>
#include <mutex>
#include <condition_variable>
#include <functional>
#include <atomic>

#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <unistd.h>

using namespace std;

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

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

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

class Pool {
public:
    typedef function<void()> Task;

    Pool() : done(false) {
        unsigned c = thread::hardware_concurrency();
        //unsigned c = 1000;
        printf("Pool() with %d threads\n", c);
        for(unsigned i = 0; i < c; ++i) {
            vt.push_back(thread(&Pool::worker, this));
        }
    }

    ~Pool() {
        done = true;
    }

    Pool(const Pool &) = delete;

    void submit(Task t) {
        sq.push(t);
    }
private:
    atomic_bool done;
    SafeQueue<Task> sq;
    vector<thread> 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\n<html><body><i>Hello!</i></body></html>";
    const char *response_400 = "HTTP/1.1 400 Bad Request\nConnection: close\nContent-Type: text/html; charset=utf-8\n\n<html><body><i>Bad Request!</i></body></html>";
    const char *response_404 = "HTTP/1.1 404 Not Found\nConnection: close\nContent-Type: text/html; charset=utf-8\n\n<html><body><i>Not Found!</i></body></html>";

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

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

    while (true) {
        n = read(cliefd, buffer, sizeof(buffer) - 1);
        if(n == -1) {
            perror("read()");
            return;
        } else if (n == 0) {
            break;
        }

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

        if(
            s[s.size() - 2] == '\r'
            && s[s.size() - 1] == '\n'
                ) {
            break;
        }
    }

    //printf("read() %s\n", s.c_str());

    response = response_400;

    for(int i = 0; i < 3 && ss; i++) {
        ss >> token;
        //printf("token %d %s\n", i, token.c_str());
        token_list.push_back(token);
    } 

    if(token_list.size() == 3 
            && token_list[0] == "GET" 
            && token_list[2].substr(0, 4) == "HTTP") {
        if(token_list[1] == "/index.html") {
            response = response_200;
        } else {
            response = response_404;
        }
    }

    while(true) {
        n = write(cliefd, response, strlen(response));
        if(n == -1) {
            perror("write()"); 
            return;
        } else if (n == 0) {
            break;
        }

        response += n;

        if(!strlen(response)) {
            break;
        }

    }

    close(cliefd);
    return;
}

int main(int argc, const char *argv[])
{
    int sockfd = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
    struct sockaddr_in servaddr;

    if(sockfd < 0) {
        perror("socket() error");
        exit(EXIT_FAILURE); 
    }

    servaddr.sin_family = AF_INET;
    servaddr.sin_port = htons(8080);
    servaddr.sin_addr.s_addr = htonl(INADDR_ANY);

    if(bind(sockfd, (struct sockaddr *)&servaddr, sizeof(servaddr)) < 0) {
        perror("bind()");
        exit(EXIT_FAILURE); 
    }


    if(listen(sockfd, 1000) < 0) {
        perror("listen()");
        exit(EXIT_FAILURE); 
    }


    struct sockaddr_storage clieaddr;
    int cliefd;
    char s[INET_ADDRSTRLEN];
    socklen_t cliesize;

    Pool p;

    while(true) {

        cliesize = sizeof(clieaddr);
        cliefd = accept(sockfd, (struct sockaddr *)&clieaddr, &cliesize);
        if(cliefd < 0) {
            perror("accept()");
            continue;
        }

        /*
         * std::thread::hardware_concurrency
         */

        inet_ntop(clieaddr.ss_family, (void *)&((struct sockaddr_in *)&clieaddr)->sin_addr, s, sizeof(s));
        printf("accept() %s\n", s);

        p.submit(bind(handle_request, cliefd));
    }

    return 0;
}
share|improve this question

2 Answers 2

up vote 4 down vote

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<mutex> 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<mutex> lk(m);
    q.push(std::forward<T>(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<mutex> 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<string> 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);
share|improve this answer
up vote 0 down vote

First, don't use using namespace std. When you do this, you are liable to get calls to ambiguously named functions, which will cause errors. Always specify which namespace you are using like this: namespace::function, as in std::cout.

You have calls to printf() in your program. In C++, you typically use std::cout << instead. This SO question discusses using printf(), and the second answer recommends avoiding printf() entirely in C++.

Additionally, you have some commented code in there, presumably because it doesn't work or because it was used for debugging. You should probably remove commented code as it doesn't do anything - especially if it is buggy.

I really like how you use spaces around your operators, and how you use braces around all your if/else statements. This makes your code easier to read and helps prevent bugs.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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