6
\$\begingroup\$

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 <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>

#include <sstream>
#include <vector>

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

using namespace std;

    const char *response_200 = "HTTP/1.1 200 OK\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\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\nContent-Type: text/html; charset=utf-8\n\n<html><body><i>Not Found!</i></body></html>";

void *handle_request(void *pcliefd) 
{
    int cliefd = *(int*)pcliefd;
    delete (int *)pcliefd;

    ssize_t n;
    char buffer[255];
    const char *response;

    n = recv(cliefd, buffer, sizeof(buffer), 0);
    if(n < 0) {
        perror("recv()");
        return 0;
    }

    buffer[n] = 0;
    //printf("recv() %s\n", buffer);

    response = response_400;

    string s(buffer), token;
    istringstream ss(s);
    vector<string> token_list;
    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;
        }
    }

    n = write(cliefd, response, strlen(response));
    if(n < 0) {
        perror("write()"); 
        return 0;
    }

    close(cliefd);
    return 0;
}

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

    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;

    while(true) {

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

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


        int *pcliefd = new int;
        *pcliefd = cliefd;
        if(true) {
            if(pthread_create(&thread, 0, handle_request, pcliefd) < 0) {
                perror("pthread_create()");
            } 
        } else {
            handle_request(pcliefd);
        }
    }

    return 0;
}
Improve this question
\$\endgroup\$
4
  • 2
    \$\begingroup\$ If you're using C++, aren't you supposed to use the C++ versions of the headers instead of the C ones (e.g. <cstdio> instead of <stdio.h>? \$\endgroup\$
    – Snowbody
    Mar 9, 2015 at 15:41
  • 3
    \$\begingroup\$ Creating a thread is relatively expensive. Create a bunch at startup and re-use them. \$\endgroup\$
    – Martin York
    Mar 9, 2015 at 16:19
  • 2
    \$\begingroup\$ If the problem is designed to demonstrate your ability to code professionally in C++, having zero comments is a serious red flag. Show you can comment as well as code. \$\endgroup\$
    – HostileFork says dont trust SE
    Mar 10, 2015 at 17:36
  • \$\begingroup\$ Sorry, but you shouldn't modify your code on Code Review if it invalidates the answers. I was trying to read the answers and didn't understand what that had to do with the code. If you want to post a revised version, then please create a follow-up question :) \$\endgroup\$
    – Morwenn
    Mar 20, 2015 at 15:36

4 Answers 4

Reset to default
7
\$\begingroup\$

  • The server announces HTTP 1.1, therefore it must support HTTP 1.1 features, most importantly pipelining (client has a right to send multiple requests over the same socket). In any case, if you chose to close the client socket immediately you should inform client with a Connection: close header.

  • You expect to consume a complete request with a single read. In a streaming world of TCP you never know how many bytes has actually been consumed. You should be prepared to get as little as one byte at a time (same goes for send/write).

    A correct way to read the request is to read it in the loop until the end of headers (i.e. an empty string) is seen.

  • Network program must be defensive. A malicious client can play tricks with your server.

    For example, it may never send anything; eventually the server would run out of file descriptors (or pthreads), resulting in denial of service. Another client may close the socket immediately upon sending a request; the server receives a SIGPIPE and dies with a core dump.

Improve this answer
\$\endgroup\$
6
\$\begingroup\$

At first I got the impression that this is actually C with new/delete and vector/stringstream to simplify the parsing.

This might be a nice test for general skills, but if you look for a C++ programmer you might want to focus more on C++ pitfalls and maybe OOP design.

Also when looking for programmers I would want to see if they keep themselves up-to-date. So testing for C++11/14 knowledge might be a good thing. So back to the code: pthreads could easily have been replaced by std::thread.

Then POSIX: while it is good to know it is also platform dependant. I would probably give the choice to use Boost. Boost is a somewhat special 3rd party library as its components tend to be included in later C++ standards (like boost::thread -> std::thread, boost::regex -> std::regex, etc.) So knowing your way with boost is actually a good thing for every C++ developer.

So this whole thing could have been done with Boost.Asio for the networking and std::thread/std::future for parallelization. So I would set the available libraries to "STL and Boost".

Other than that I noticed that write is used. Actually the counterpart of recv is send.

Improve this answer
\$\endgroup\$
0
\$\begingroup\$

This might not be the best idea (google up arguments and decide for yourself):

using namespace std;

I presume this is a typo:

if(true) {

Nitpicking: ++i is preferrable

for(int i = 0; i < 3 && ss; i++) {

You are basically saying that your server implements also version HTTPxyzandwhatnot of HTTP. Might be against specs.

&& token_list[2].substr(0, 4) == "HTTP") {

UPDATE: To be ok with requirement 4. "Valid http status codes and headers" maybe you should also consider HTTP 500 status. Be aware that STL stuff can (usually rarely but still) throw exceptions (bad_alloc etc.).

Improve this answer
\$\endgroup\$
9
  • \$\begingroup\$ In general yes of course. In context of that int in for loop nope. Pre-increment version is always faster or same as post-increment depending on your compiler. Refer to: stackoverflow.com/questions/24886/… \$\endgroup\$
    – Jan Korous
    Mar 12, 2015 at 21:34
  • \$\begingroup\$ @SirPython: I disagree ++i is preferable. Not because it provides any performance improvement in this situation. But because there are situations where it does. Thus to main consistency across your code base you need to conform to a style. To have a style that always provides the most efficient version is the correct way to go. It also allows you to change types without having to change code and still have the most efficient version. You can then use i++ in those rare situations where you want to use its specific properties. \$\endgroup\$
    – Martin York
    Mar 22, 2015 at 0:55
  • \$\begingroup\$ @SirPython: Again disagree. A style is what you use consistently across all your code. In C++ it is preferable to use ++i consistently as it will always get you the best performance version. The article you refer to is about performance of int and operator++ but operator++ can be applied to all classes and the difference between is the two is usually a copy of the object which can be costly. \$\endgroup\$
    – Martin York
    Mar 22, 2015 at 1:06
  • \$\begingroup\$ If you are not concerned about speed then again for consistency (which is my point) you should be using ++i. Since you want performant code in all situations you don't want to change the code just because you change the underlying type (as that cause maintenance problems). As such a consistent style that always works is ++i. \$\endgroup\$
    – Martin York
    Mar 22, 2015 at 1:09
  • 1
    \$\begingroup\$ OMG guys you picked the single by far least significant part of my comments and started an argument. Are you serious? :-) \$\endgroup\$
    – Jan Korous
    Mar 23, 2015 at 6:28
0
\$\begingroup\$

use c++ headers (as somebody already said but can't be mentioned enough)

in handle_request you delete what comes in, it is a bit unusual to do it like that because the caller of the function has no way of knowing (from the prototype/name of the function) that you are doing a delete on argument. Use instead a std::unique_ptr as argument making it clear that ownership is taken over i.e. std::move (even though i am not sure why you allocate an int on the heap and pass it, instead of just passing an int by value)

you should use vector instead of normal c arrays

e.g. char buffer[255] can be replaced with std::vector<char> buffer(255);

use const in a C++ manner, writing const char* response is writing it in a C-legacy style, instead write char * const; (const always referring to the item left of const i.e. in this case the pointer *)

you normally don't use printf in c++ programs, instead use cout ... even for debug output

always initialize all variables, in particular the POD ones and structs.

try....catch is good to have in main to exit gracefully if something goes wrong.

exit(EXIT_FAILURE) in main could be replaced with return EXIT_FAILURE;

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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